Skip to content

Allow triggerRequest and signalCancel to be coalesced #462

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

Merged

Conversation

jroper
Copy link
Contributor

@jroper jroper commented Jul 3, 2019

@jroper jroper mentioned this pull request Jul 4, 2019
stage.expectRequest();
stage.puppet().signalCancel();
stage.expectCancelling();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we expect the request before signalling cancel, preventing the coalescing of the request and cancel, and by expecting cancelling before signalling next, we ensure that signalling next does test what this test says it tests.

Copy link

Choose a reason for hiding this comment

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

Sounds reasonable and less prone to race conditions.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. the behavior you described sounds like an improvement.

@viktorklang
Copy link
Contributor

@reactive-streams/contributors Feedback on this?

@akarnokd
Copy link
Contributor

I'm not sure. Is this something because of Akka Stream's sequential signal count and mixing requests with cancellation in the same actor message queue?

@viktorklang
Copy link
Contributor

@akarnokd I think @jroper will be able to answer that. That aside though, the importance to the TCK is to be able to enforce the spec, so it is more about making sure of that.

@viktorklang
Copy link
Contributor

@jroper @akarnokd @reactive-streams/contributors Let's make some progress here so we can release the first 1.0.3 RC. Feedback encouraged

@Scottmitch
Copy link
Contributor

@viktorklang - can we merge this, cut an RC, and collect comments when folks kick the tires on the RC?

@viktorklang
Copy link
Contributor

@Scottmitch Agreed—people have had ample time to respond as is. Will tackle this on Monday

@viktorklang viktorklang merged commit f9493e3 into reactive-streams:master Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants