SpliceFlatStreamToMetaSingle: propagate cancel when races with data
#3036
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
idelpivnitskiy
commented
Aug 9, 2024
| @@ -67,6 +68,7 @@ | |||
| import static org.hamcrest.Matchers.startsWith; | |||
| import static org.junit.jupiter.api.Assertions.assertThrows; | |||
|
|
|||
| @Disabled // FIXME: remove before merging | |||
There was a problem hiding this comment.
This PR is blocked by awaiting a fix for Single.ambWith(...) operator
| if (maybePayloadSubUpdater.compareAndSet(this, null, CANCELED)) { | ||
| final Object current = maybePayloadSubUpdater.getAndUpdate(this, | ||
| curr -> curr == null || curr == PENDING ? CANCELED : curr); | ||
| if (current == null || current == PENDING) { |
There was a problem hiding this comment.
Please carefully review the updated state machine and existing tests that we have. If you see a use-case not covered by the tests, will appreciate new ideas.
bryce-anderson
approved these changes
Aug 9, 2024
...icetalk-http-netty/src/main/java/io/servicetalk/http/netty/SpliceFlatStreamToMetaSingle.java
Outdated
Show resolved
Hide resolved
Scottmitch
approved these changes
Aug 12, 2024
...icetalk-http-netty/src/main/java/io/servicetalk/http/netty/SpliceFlatStreamToMetaSingle.java
Outdated
Show resolved
Hide resolved
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:
Cancellation of the single may race with delivering the first element. If
cancelwins, we already deliverCancellationExceptionif someone subscribes to the payload publisher. However, ifcancelarrives afterdataSubscriber.onSuccess(...)and before someone subscribes to the payload publisher, it's a strong indicator that there was a race anddataSubscriberis not interested in it anymore. Therefore, we should pessimistically assume that nobody will ever subscribe to the payload publisher, cancel upstream subscription, and deliverCancellationExceptionis someone still subscribes.Modifications:
nullandPENDINGstates if received data cancel;parent.packer.applyto assert that this path happens only ifCANCELED;CancellationExceptionis someone still subscribes to the payload publisher afterCANCELED;dataSubscriber.onErrorifmetaSeenInOnNext == truebecause it's already terminated (CANCELEDdoes not play any role here);payloadSubscriberis already terminated (EMPTY_COMPLETED_DELIVERED);Result:
Network resources receive
cancelsignal and clean up connection state.Note that the original version of this operator had this behavior, but it was changed in 38e2c67
We don't have a full PR for that commit with review comments. Based on the commit description I believe it was fixing a different problem and the change in cancel propagation behavior for
PENDINGstate is likely a side-effect of introducing a new state.