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

Fixed early response warning on chunked requests #9077

Merged
merged 1 commit into from Mar 8, 2019

Conversation

Projects
None yet
4 participants
@jroper
Copy link
Member

commented Mar 8, 2019

The takeWhile(_.isLastChunk) causes the downstream sink to immediately receive a completion, while terminating upstream so that from Akka HTTPs perspective, the full request body hasn't been consumed yet. Due to one or the other of these, when the response does get sent, Akka HTTP outputs a warning saying the response is being sent before the request has been consumed.

This replaces it with a filter(_.isLastChunk), so that no termination signals get introduced other than the one coming from Akka HTTP itself, and so consequently avoids the early termination error, meanwhile it has the same effect as takeWhile since the last chunk is always the last element in the stream.

Fixed early response warning on chunked requests
The `takeWhile(_.isLastChunk)` causes the downstream sink to immediately
receive a completion, while terminating upstream so that from Akka HTTPs
perspective, the full request body hasn't been consumed yet. Due to one
or the other of these, when the response does get sent, Akka HTTP
outputs a warning saying the response is being sent before the request
has been consumed.

This replaces it with a `filter(_.isLastChunk)`, so that no termination
signals get introduced other than the one coming from Akka HTTP itself,
and so consequently avoids the early termination error, meanwhile it has
the same effect as `takeWhile` since the last chunk is always the last
element in the stream.
@jroper

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2019

Here's the issue reported to Akka HTTP, not sure if it should be considered an Akka HTTP bug or not: akka/akka-http#2455

@dwijnand
Copy link
Member

left a comment

Doesn't conflict with the original intent in 114028c.

A test capturing the corrected behaviour is always well received, but LGTM as is too.

Thank you, James.

Team: is from a support case, so we might want to backport to 2.6 and at some point cut a bugfix release (it's just avoiding a warning, so no rush).

@jroper

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2019

I suspect we already have the tests there, and the warning is probably being output, we just have been ignoring it.

@jroper

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2019

Here's one such test:

https://travis-ci.com/playframework/playframework/jobs/182430143#L2621

[info] WSClient@java should
[info]   + make GET Requests (136 ms)
[info]   + make DELETE Requests (127 ms)
[info]   + use queryString in url (139 ms)
[info]   + use user:password in url (161 ms)
[info]   + reject invalid query string (100 ms)
[info]   + reject invalid user password string (116 ms)
[info]   + consider query string in JSON conversion (155 ms)
[info]   + get a streamed response (135 ms)
[WARN] [03/05/2019 15:24:28.129] [application-akka.actor.default-dispatcher-3] [akka.actor.ActorSystemImpl(application)] Sending an 2xx 'early' response before end of request was received... Note that the connection will be closed after this response. Also, many clients will not read early responses! Consider only issuing this response after the request data has been completely read!
[info]   + streaming a request body (105 ms)
[info]   + streaming a request body with manual content length (110 ms)

You can see that log message gets output immediately before the streaming a request body test passes, that test doesn't send a content length (because the next one says it does), so is using chunked requests, and so triggers the warning. But since the primary effect of the bug is a warning in the logs, it's not that straight forward to test. We can see if this PR outputs the same warning or not.

@mergify mergify bot merged commit d247b27 into playframework:master Mar 8, 2019

3 checks passed

Mergify — Summary 2 rules match
Details
Travis CI - Pull Request Build Passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@renatocaval renatocaval removed the in progress label Mar 8, 2019

@dwijnand

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

2.7.x 3decd04
2.6.x babfa7c

dwijnand added a commit that referenced this pull request Mar 8, 2019

Fixed early response warning on chunked requests (#9077)
The `takeWhile(_.isLastChunk)` causes the downstream sink to immediately
receive a completion, while terminating upstream so that from Akka HTTPs
perspective, the full request body hasn't been consumed yet. Due to one
or the other of these, when the response does get sent, Akka HTTP
outputs a warning saying the response is being sent before the request
has been consumed.

This replaces it with a `filter(_.isLastChunk)`, so that no termination
signals get introduced other than the one coming from Akka HTTP itself,
and so consequently avoids the early termination error, meanwhile it has
the same effect as `takeWhile` since the last chunk is always the last
element in the stream.

(cherry picked from commit d247b27)

dwijnand added a commit that referenced this pull request Mar 8, 2019

Fixed early response warning on chunked requests (#9077)
The `takeWhile(_.isLastChunk)` causes the downstream sink to immediately
receive a completion, while terminating upstream so that from Akka HTTPs
perspective, the full request body hasn't been consumed yet. Due to one
or the other of these, when the response does get sent, Akka HTTP
outputs a warning saying the response is being sent before the request
has been consumed.

This replaces it with a `filter(_.isLastChunk)`, so that no termination
signals get introduced other than the one coming from Akka HTTP itself,
and so consequently avoids the early termination error, meanwhile it has
the same effect as `takeWhile` since the last chunk is always the last
element in the stream.

(cherry picked from commit d247b27)
@mkurz

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

Does this fix #8824 now?

@jroper

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

@mkurz I think it should.

@jroper jroper deleted the jroper:fix-chunked-request-warning branch Mar 18, 2019

@dwijnand dwijnand added this to the Play 2.7.1 milestone Mar 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.