Re: [jdk17] RFR: 8268342: java/foreign/channels/TestAsyncSocketChannels.java fails with "IllegalStateException: This segment is already closed"
On Fri, 11 Jun 2021 16:27:07 GMT, Daniel Fuchs wrote: >> There is the possibility for a race in the test, where the outbound socket >> buffer is still being filled, after 5 seconds, when the main test thread >> tries to close the resource scope. >> >> The test has been updated to set the send/receive buffer sizes in an >> attempt/hint to limit the accepted/connected socket buffer sizes. Actual >> buffer sizes in use will likely be larger due to TCP auto-tuning, but the >> hint typically reduces the overall scaled sizes. This is primarily to >> stabilize outstanding write operations. Additionally, to ameliorate the >> wait-for-writebuf-to-fill situation, the dumb sleep 5secs has been replaced >> with a heuristic that checks that the number of bytes written quiesces. With >> these changes, the test successfully passes several thousands of runs. > > test/jdk/java/foreign/channels/TestAsyncSocketChannels.java line 283: > >> 281: readNBytes(asc2, bytesWritten.get()); >> 282: assertTrue(scope.isAlive()); >> 283: awaitOutstandingWrites(outstandingWriteOps); > > An alternative would be to delegate the call to latch.countDown() to the > TestHandler subclass, and call it at the end of completed() only when > outstandingWriteOps.decrementAndGet() == 0; As it stands the latch is > released with the first call to `completed` (instead of being released with > the last one) - and that's what makes this method necessary. Strictly speaking it wasn't necessary to touch this area of code to resolve the failures that we're seeing - I decided to refactor this into a separate method to improve readability of the test logic. Here again, awaiting for completion is not strictly necessary, just good practice to ensure that all threads and operations associated with the test scenario complete before the next scenario. I'll see if this can be simplified as you suggest. - PR: https://git.openjdk.java.net/jdk17/pull/30
Re: [jdk17] RFR: 8268342: java/foreign/channels/TestAsyncSocketChannels.java fails with "IllegalStateException: This segment is already closed"
On Fri, 11 Jun 2021 15:26:50 GMT, Chris Hegarty wrote: > There is the possibility for a race in the test, where the outbound socket > buffer is still being filled, after 5 seconds, when the main test thread > tries to close the resource scope. > > The test has been updated to set the send/receive buffer sizes in an > attempt/hint to limit the accepted/connected socket buffer sizes. Actual > buffer sizes in use will likely be larger due to TCP auto-tuning, but the > hint typically reduces the overall scaled sizes. This is primarily to > stabilize outstanding write operations. Additionally, to ameliorate the > wait-for-writebuf-to-fill situation, the dumb sleep 5secs has been replaced > with a heuristic that checks that the number of bytes written quiesces. With > these changes, the test successfully passes several thousands of runs. Looks good but you could make the logic simpler by calling latch.countDown() in the last call to completed(), instead of doing it in the first one. test/jdk/java/foreign/channels/TestAsyncSocketChannels.java line 283: > 281: readNBytes(asc2, bytesWritten.get()); > 282: assertTrue(scope.isAlive()); > 283: awaitOutstandingWrites(outstandingWriteOps); An alternative would be to delegate the call to latch.countDown() to the TestHandler subclass, and call it at the end of completed() only when outstandingWriteOps.decrementAndGet() == 0; As it stands the latch is released with the first call to `completed` (instead of being released with the last one) - and that's what makes this method necessary. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/30
[jdk17] RFR: 8268342: java/foreign/channels/TestAsyncSocketChannels.java fails with "IllegalStateException: This segment is already closed"
There is the possibility for a race in the test, where the outbound socket buffer is still being filled, after 5 seconds, when the main test thread tries to close the resource scope. The test has been updated to set the send/receive buffer sizes in an attempt/hint to limit the accepted/connected socket buffer sizes. Actual buffer sizes in use will likely be larger due to TCP auto-tuning, but the hint typically reduces the overall scaled sizes. This is primarily to stabilize outstanding write operations. Additionally, to ameliorate the wait-for-writebuf-to-fill situation, the dumb sleep 5secs has been replaced with a heuristic that checks that the number of bytes written quiesces. With these changes, the test successfully passes several thousands of runs. - Commit messages: - initial changes Changes: https://git.openjdk.java.net/jdk17/pull/30/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=30=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268342 Stats: 52 lines in 1 file changed: 46 ins; 4 del; 2 mod Patch: https://git.openjdk.java.net/jdk17/pull/30.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/30/head:pull/30 PR: https://git.openjdk.java.net/jdk17/pull/30
