Skip to content
New issue

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

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

Already on GitHub? # to your account

Test: Inspect chunked responses during termination #3139

Merged
merged 6 commits into from
Jun 3, 2020

Conversation

ennru
Copy link
Member

@ennru ennru commented May 8, 2020

Tests that a chunked HTTP response stream fails when the server terminates.

The Akka HTTP client reacts as it should with "EntityStreamException: Entity stream truncation. The HTTP parser was receiving an entity when the underlying connection was closed unexpectedly."

#1210 (comment) made me curious how a client will see server termination on a chunked HTTP response stream.

I expect this test to be flaky, so I'm not sure if it is worth merging.

References #1210

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed validating PR that is currently being validated by Jenkins labels May 8, 2020
@akka-ci
Copy link

akka-ci commented May 8, 2020

Test PASSed.

@jrudolph
Copy link
Contributor

We are missing such a test indeed, mainly because we don't seem to verify yet that streams that are still ongoing are really aborted in time. What we should strive for is that the connection is aborted, otherwise, responses that use HttpEntity.CloseDelimited will see a truncated body without an error.

The test case seems a bit fragile since it uses the same ActorSystem for server and client. We should change that as well, because we might want to add Http().shutdownConnectionPools to the coordinated shutdown as well and that might interfere with the testing logic.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels May 11, 2020
@akka-ci
Copy link

akka-ci commented May 11, 2020

Test FAILed.

Pull request validation report

Failed Test Suites

Test result for 'akka-http-core / Pr-validation / ./ executeTests'

[info] ScalaTest
[info] Run completed in 4 minutes, 43 seconds.
[info] Total number of tests run: 1052
[info] Suites: completed 76, aborted 0  Scopes: pending 1
[info] Tests: succeeded 1048, failed 4, canceled 1, ignored 2, pending 55
[info] *** 4 TESTS FAILED ***
[error] Failed: Total 1052, Failed 4, Errors 0, Passed 1048, Ignored 2, Canceled 1, Pending 55
[error] Failed tests:
[error] 	akka.http.scaladsl.ClientServerSpec

@jrudolph
Copy link
Contributor

Unrelated failure that will be fixed with #3145.

@jrudolph
Copy link
Contributor

PLS BUILD

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels May 12, 2020
@akka-ci
Copy link

akka-ci commented May 12, 2020

Test PASSed.

try {
val termination = serverBinding.terminate(hardDeadline = 50.millis)
response.request(20)
// local testing shows the stream fails long after the 50 ms deadline
Copy link
Contributor

Choose a reason for hiding this comment

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

So this test shows the deadline isn't actually enforced for streams like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

(I guess that might not be a big problem since the actorsystem shutdown will shut down the materializer and fail the stream anyway?)

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 banging my head against this a bit... What I'm seeing is that the GracefulTerminationStage does something after those 50ms but that does not kill the connection immediately (especially not the response stream). My hypothesis is, since the termination stage does not monitor response entity streams, it does something which will only indirectly stop the connection at some point. In any case, it does not cancel connection but complete it, which will still keep proper semantics in this case (because chunked encoding can detect truncation) but not in the below one.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels May 15, 2020
@akka-ci akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR that is currently being validated by Jenkins labels May 15, 2020
@ennru
Copy link
Member Author

ennru commented May 15, 2020

What we should strive for is that the connection is aborted, otherwise, responses that use HttpEntity.CloseDelimited will see a truncated body without an error.

Yes, I added a test for that and the client doesn't understand that it's over. The test is ignored for now.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels May 15, 2020
@akka-ci akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR that is currently being validated by Jenkins labels May 15, 2020
@akka-ci
Copy link

akka-ci commented May 15, 2020

Test PASSed.

Copy link
Contributor

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

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

LGTM, I'm fine with merging, I debugged the remaining issue a bit and will create a ticket to follow up.

@jrudolph
Copy link
Contributor

jrudolph commented Jun 3, 2020

I debugged the remaining issue a bit and will create a ticket to follow up.

#3209

@ennru ennru requested a review from raboof June 3, 2020 12:09
@akka-ci akka-ci added validating PR that is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Jun 3, 2020
Copy link
Contributor

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Good to have this behavior covered with a test, indeed let's follow up on #3209 in a separate PR

response.request(20)
// local testing shows the stream fails long after the 50 ms deadline
response.expectNext().utf8String should ===("reply2")
response.expectNext().utf8String should ===("reply3")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove a couple of these as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@akka-ci akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR that is currently being validated by Jenkins labels Jun 3, 2020
@akka-ci
Copy link

akka-ci commented Jun 3, 2020

Test PASSed.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Jun 3, 2020
@akka-ci
Copy link

akka-ci commented Jun 3, 2020

Test PASSed.

@ennru ennru added this to the 10.2.0 milestone Jun 3, 2020
@ennru ennru changed the title Inspect chunked responses during termination Test: Inspect chunked responses during termination Jun 3, 2020
@ennru ennru merged commit ff00686 into akka:master Jun 3, 2020
@ennru ennru deleted the termination-chunked branch June 3, 2020 12:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants