Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #5409 - HttpClient fails intermittently with "Invalid response … #5449

Merged

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Oct 13, 2020

…state TRANSIENT".

The problem was a race condition during content decoding.
Since decoding needs to be done in a loop, the condition to loop is to
check whether there is demand for the next chunk of decoded content.

Checking for demand also sets the stalled flag, and this must be done
only after the response state has been set back to CONTENT.
Unfortunately this was not done in the decoding loop.

The fix is to always update the response state in the decoding loop.

Signed-off-by: Simone Bordet simone.bordet@gmail.com

…state TRANSIENT".

The problem was a race condition during content decoding.
Since decoding needs to be done in a loop, the condition to loop is to
check whether there is demand for the next chunk of decoded content.

Checking for demand also sets the stalled flag, and this must be done
only after the response state has been set back to CONTENT.
Unfortunately this was not done in the decoding loop.

The fix is to always update the response state in the decoding loop.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested a review from lorban October 13, 2020 21:19
@sbordet sbordet added this to In progress in Jetty 9.4.33 via automation Oct 13, 2020
Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM besides the minor proposal for a signature change. Good to go as-is if you don't like my suggestion.

@@ -614,15 +565,42 @@ public boolean abort(HttpExchange exchange, Throwable failure)
}
}

private boolean updateResponseState(ResponseState from1, ResponseState from2, ResponseState to)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fond of this method's signature, what do you think of that one instead?

private boolean updateResponseState(EnumSet<ResponseState> from, ResponseState to)

that would make the callers look like this:

if (updateResponseState(EnumSet.of(ResponseState.HEADERS, ResponseState.CONTENT), ResponseState.TRANSIENT))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It allocates 😃

@sbordet sbordet merged commit 7bfca25 into jetty-9.4.x Oct 14, 2020
Jetty 9.4.33 automation moved this from In progress to Done Oct 14, 2020
@sbordet sbordet deleted the jetty-9.4.x-5409-invalid_response_state_transient branch October 14, 2020 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Jetty 9.4.33
  
Done
Development

Successfully merging this pull request may close these issues.

HttpClient fails intermittently with "Invalid response state TRANSIENT"
2 participants