JAVA-6144 increase wait time for thread pool shutdown#1920
JAVA-6144 increase wait time for thread pool shutdown#1920strogiyotec merged 2 commits intomongodb:mainfrom
Conversation
| @@ -71,6 +80,6 @@ void testExternalExecutorWasShutDown(final boolean tlsEnabled) throws Interrupte | |||
| // ignored | |||
| } | |||
|
|
|||
| assertTrue(executorService.awaitTermination(100, TimeUnit.MILLISECONDS)); | |||
| assertTrue(executorService.awaitTermination(2, TimeUnit.SECONDS)); | |||
There was a problem hiding this comment.
Let's remove the code comment. My thoughts are below.
Your observed that most of the time was spent on a specific activity, which you described. That observation depends on the circumstances, not a general truth.
The MongoClient.close is asynchronous, and in this PR you increased the duration of time the test waits for the subset of the close work to be completed. The previous duration of 100 ms was arbitrary, and the new duration is also arbitrary (it's not a fixed number you can compute that is guaranteed to be enough in all circumstances). The only criteria - it should be long enough for the test to seemingly always pass (thank you for experimenting and confirming this!).
I think, you investigation was useful, it satisfied your curiosity, and a PR-level comment may be useful to satisfy the curiosity of a PR reviewer, but the code-level comment seems strictly harmful to me.
There was a problem hiding this comment.
hey Valentin , thanks for the feedback
- I removed the javadoc from test method
- Call
closedirectly instead of try-with-resources - Added JIRA link to the PR description
| @@ -71,6 +80,6 @@ void testExternalExecutorWasShutDown(final boolean tlsEnabled) throws Interrupte | |||
| // ignored | |||
| } | |||
There was a problem hiding this comment.
[unrelated to this PR]
Let's make this code more expressive, straightforward, and shorter:
new SyncMongoClient(mongoClientSettings).close();As far as I can tell, there is no reason to use the try-with-resources statement in this case.
|
@strogiyotec, could you please specify JAVA-6144 in the ticket description? This way a reader may click on it to navigate to the ticket, and Jira will automatically link the PR to the ticket. |
JAVA-6144
The test case was flaky
Here is 1 example of failing test
The test only fails for NoTls variant
The test itself calls
closeon the mongo client and expects underlyingthreadpoolto await termination for less than 100MSAfter looking at the code I found that the main cause is in
AsynchronousSocketChannelStreamFactoryFactoryspecifically howcloseworks thereThe group is
AsynchronousChannelGroupand it's created inStreamFactoryHelperhereand from the java doc https://docs.oracle.com/javase/8/docs/api/java/nio/channels/AsynchronousChannelGroup.html
Basically once we call
shutdownon the channel this operation is non blocking and only marks executor as being shutdown , the gap betweenshutdownandawaitTerminationwithin a test case is small and occasionally failsTo test this using a patch
tlsEnabled=falseRepetableTestto run 100 times (that's whytlsEnabledwas hardocded to false,RepeatedTestdoesn't supportValueSource)Another solution I could think of is to use
shutDownNowhowever the shutdown will be blocking