Racy cancel from transport does not always cancel the message body
#2369
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
Motivation: When `cancel` from the transport races with HTTP message completion, we can receive `cancel` after `Single<HttpMetaData>` completes but before we subscribed to the `messageBody` publisher. In this case, we won't subscribe to the `messageBody` because we use `concat` operator. This does not allow `BeforeFinallyHttpOperator` to observe a terminal event. Race is possible when offloading is enabled. `cancel` comes from an IO thread, HTTP meta-data completes on offloaded thread. Modifications: - For every case when we concat a completed source with the message body add `afterFinally` to guarantee propagation of the cancel for the entire flat HTTP stream; Result: `HttpLifecycleObserverTest.testClientCancelsRequestBeforeResponse()` does not race anymore.
Scottmitch
reviewed
Sep 24, 2022
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HeaderUtils.java
Outdated
Show resolved
Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HeaderUtils.java
Outdated
Show resolved
Hide resolved
Scottmitch
reviewed
Sep 30, 2022
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HeaderUtils.java
Outdated
Show resolved
Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/NettyHttpServer.java
Outdated
Show resolved
Hide resolved
|
@Scottmitch rebased after your changes, ready for review |
Scottmitch
approved these changes
Oct 4, 2022
There was a problem hiding this comment.
Is the test failure related at all?
https://github.com/apple/servicetalk/runs/8686381887
HttpTransportObserverTest.serverHandlerError(HttpProtocol) [1] protocol=HTTP_1
Wanted but not invoked:
serverWriteObserver.itemFlushed();
-> at io.servicetalk.http.netty.HttpTransportObserverTest.testRequestResponse(HttpTransportObserverTest.java:243)
However, there were exactly 4 interactions with this mock:
serverWriteObserver.requestedToWrite(64L);
-> at io.servicetalk.transport.api.CatchAllTransportObserver$CatchAllWriteObserver.lambda$requestedToWrite$0(CatchAllTransportObserver.java:255)
serverWriteObserver.itemReceived(
HTTP/1.1 500 Internal Server Error
);
-> at io.servicetalk.transport.api.CatchAllTransportObserver$CatchAllWriteObserver.lambda$itemReceived$1(CatchAllTransportObserver.java:260)
serverWriteObserver.onFlushRequest();
-> at io.servicetalk.transport.api.CatchAllTransportObserver.safeReport(CatchAllTransportObserver.java:306)
serverWriteObserver.itemWritten(
HTTP/1.1 500 Internal Server Error
);
-> at io.servicetalk.transport.api.CatchAllTransportObserver$CatchAllWriteObserver.lambda$itemWritten$2(CatchAllTransportObserver.java:270)
at app//io.servicetalk.http.netty.HttpTransportObserverTest.testRequestResponse(HttpTransportObserverTest.java:243)
at app//io.servicetalk.http.netty.HttpTransportObserverTest.serverHandlerError(HttpTransportObserverTest.java:213)
| // with non-replayable messageBody to retry. | ||
| if (emptyMessageBody(request, messageBody)) { | ||
| flatRequest = flatEmptyMessage(connectionContext().protocol(), request, messageBody, | ||
| /* propagateCancel */ false); |
There was a problem hiding this comment.
nit: /* propagateCancel */ -> do we need this? I think it's safe to assume folks will be able to follow the code in IDE or preferred method to understand parameter meanings (e.g. expectation for java code). The more interesting part is explaining why (e.g. client doesn't need it, but server does).
There was a problem hiding this comment.
It doesn't hurt to have it either. I frequently read code on GitHub without IDE and share a direct link to interesting places.
|
The test failyre is unrelated, it's known flaky test #2040 |
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.
Motivation:
When
cancelfrom the server's transport races with HTTP message completion, we can receivecancelafterSingle<HttpResponseMetaData>completes but before we subscribe to themessageBodypublisher. In this case, we won't subscribe to themessageBodybecause we useconcatoperator. This does not allowBeforeFinallyHttpOperatorto observe a terminal event. The race is possible when offloading is enabled.cancelcomes from an IO thread, HTTP meta-data completes on the offloaded thread.Modifications:
concatPropagateCanceloperator to guarantee propagation of the cancel for the entire flat HTTP stream;Result:
HttpLifecycleObserverTest.testClientCancelsRequestBeforeResponse()does not race anymore.