[UNDERTOW-1377] Test to reproduce concurrent dispatch failure #657
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Linux Build 2733 outcome was FAILURE using a merge of 1393a97 Failed tests |
|
Windows Build 2486 outcome was FAILURE using a merge of 1393a97 Failed tests |
|
Linux Build 2734 outcome was FAILURE using a merge of c32831f Failed tests |
|
Windows Build 2487 outcome was FAILURE using a merge of c32831f Failed tests |
|
Linux Build 2735 outcome was FAILURE using a merge of c32831f Failed tests |
|
Windows Build 2488 outcome was FAILURE using a merge of c32831f Failed tests |
carterkozak
commented
Jul 12, 2018
| @@ -241,11 +248,18 @@ private void resumeReads(boolean wakeup) { | |||
| if(anyAreSet(state, FLAG_DATA_TO_UNWRAP) || wakeup || unwrappedData != null) { | |||
| runReadListener(true); | |||
| } else { | |||
| delegate.getSourceChannel().resumeReads(); | |||
| delegateSourceResumeReads(); | |||
There was a problem hiding this comment.
Invoking resumeReads off the IO thread here causes the failure.
|
Linux Build 2737 outcome was UNKNOWN using a merge of 77c493f |
|
Windows Build 2490 outcome was UNKNOWN using a merge of 77c493f |
Client gets a socket timeout where the server is unaware of an active request. This is only reproducible in parallel AND with an exchange.dispatch. I am unable to reproduce this failure without TLS.
Unsure if this is necessary. Previous commit fixes the issue that I can reproduce, it stands to reason that all other invocations should run on the IO thread as well.
carterkozak
force-pushed
the
ssl_dispatch_failure
branch
from
77c493f
to
42f35ab
Compare
|
Linux Build 2739 outcome was FAILURE using a merge of 42f35ab Failed tests |
|
Windows Build 2492 outcome was FAILURE using a merge of 42f35ab Failed tests |
|
Thanks for the test case, but I don't think this is the correct fix (although the fact that this made the test passed gave me a big clue as to what the actual problem was). In SslReadReadyHandler there is a suspendReads() that is in the wrong place, and was happening after the read listener was invoked. In some circumstances this could happen after the exchange was completed by the work thread, which means the channel ended up suspended when it should be resumed to read the next request. |
|
Ah, that makes sense, thanks @stuartwdouglas! Once the fix is pushed is there any chance you could tag a release? I've run into this issue on production systems. |
|
Looks like this test has been merged to master |
|
2.0.11.Final is tagged |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Client gets a socket timeout where the server is unaware of
an active request. This is only reproducible in parallel AND
with an exchange.dispatch.
I am unable to reproduce this failure without TLS.