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

NettyChannelPublisher cancel active subscriber should terminate #976

Merged
merged 4 commits into from Mar 26, 2020

Conversation

@Scottmitch
Copy link
Member

Scottmitch commented Mar 23, 2020

Motivation:
NettyChannelPublisher allows for resubscribes, and delivers queued data to the new Subscriber. If the previous Subscriber consumed some data and cancelled this may lead to the new Subscriber receiveing partial data from the last request.

Modifications:

  • NettyChannelPublisher should discard any pending data, and deliver an error to new Subscribers if a previously active Subscriber cancels.

Result:
Resubscribes to NettyChannelPublisher won't get partial data for previous requests, like in the case of client pipelinining.

Motivation:
NettyChannelPublisher allows for resubscribes, and delivers queued data to the new Subscriber. If the previous Subscriber consumed some data and cancelled this may lead to the new Subscriber receiveing partial data from the last request.

Modifications:
- NettyChannelPublisher should discard any pending data, and deliver an error to new Subscribers if a previously active Subscriber cancels.

Result:
Resubscribes to NettyChannelPublisher won't get partial data for previous requests, like in the case of client pipelinining.
@Scottmitch Scottmitch requested review from NiteshKant and idelpivnitskiy Mar 23, 2020
@Scottmitch Scottmitch force-pushed the Scottmitch:ncp_queue_cancel branch from 1d9adb8 to 5ebb001 Mar 23, 2020
@Scottmitch Scottmitch requested a review from NiteshKant Mar 23, 2020
Copy link
Member

NiteshKant left a comment

This looks good as-is but feel free to add the test before merging

@Scottmitch Scottmitch requested a review from NiteshKant Mar 25, 2020
@Scottmitch

This comment has been minimized.

Copy link
Member Author

Scottmitch commented Mar 25, 2020

@NiteshKant - ptal as some interesting changes made in followup

Copy link
Member

NiteshKant left a comment

💯

@Scottmitch Scottmitch merged commit afe0a23 into apple:master Mar 26, 2020
3 checks passed
3 checks passed
pull request validation (jdk11) Build finished.
Details
pull request validation (jdk8) Build finished.
Details
pull request validation (quality) Build finished.
Details
@Scottmitch Scottmitch deleted the Scottmitch:ncp_queue_cancel branch Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.