Wait for demand before signaling onNext #441
Conversation
Fixes reactive-streams#277. Fixes two whitebox subscriber tests where they weren't waiting for demand before signaling onNext.
| @@ -267,6 +267,7 @@ public void required_spec208_mustBePreparedToReceiveOnNextSignalsAfterHavingCall | |||
| public void run(WhiteboxTestStage stage) throws InterruptedException { | |||
| stage.puppet().triggerRequest(1); | |||
| stage.puppet().signalCancel(); | |||
| stage.expectRequest(); | |||
There was a problem hiding this comment.
Should be there an unit test that failed before this change?
There was a problem hiding this comment.
Thinking about this some more, shouldn't the expectRequest be before the signalCancel?
There was a problem hiding this comment.
Also could it be possible, and legal, that a request followed by a cancel is coalesced into just a cancel?
There was a problem hiding this comment.
Should be there an unit test that failed before this change?
I'll add one.
Thinking about this some more, shouldn't the
expectRequestbe before thesignalCancel?:
I don't think it matters, in fact the reason I put it in this order is that I think this order makes it more likely that the test will test what it's intending to test. Triggering a request, waiting for that request to come, then signalling cancel, then immediately sending the next message, means that there's a high chance this will pass due to signal not being handled yet. However, waiting for the request after signalling cancel increases the likelihood that the cancel signal has been handled before we emit the next element, thereby increasing the likelihood that the test will actually test what it says its testing (that elements emitted after a cancel signal is handled won't cause an error).
Also could it be possible, and legal, that a request followed by a cancel is coalesced into just a cancel?
On a subscription, yes, that's legal and possible, but this isn't a subscription, this is the TCK puppet, and the TCK defines how implementations of the puppet are are to behave. It requires that the request signal does result in a request for demand to the subscription, regardless of what signal comes immediately after. And when these signals get forwarded onto the subscription, we know the subscription won't coalesce the signals because that is a subscription that the TCK has implemented itself in ManualPublisher, and it doesn't coalesce signals, it captures them all for assertions to be run later, assertions like expectRequest.
There was a problem hiding this comment.
However, waiting for the request after signalling cancel increases the likelihood that the cancel signal has been handled before we emit the next element, thereby increasing the likelihood that the test will actually test what it says its testing (that elements emitted after a cancel signal is handled won't cause an error).
Since it is, as you say, not a full solution, may I suggest that it makes sense to add that as a comment, since it is non-apparent as to its intent?
There was a problem hiding this comment.
On a subscription, yes, that's legal and possible, but this isn't a subscription, this is the TCK puppet, and the TCK defines how implementations of the puppet are are to behave. It requires that the request signal does result in a request for demand to the subscription, regardless of what signal comes immediately after. And when these signals get forwarded onto the subscription, we know the subscription won't coalesce the signals because that is a subscription that the TCK has implemented itself in
ManualPublisher, and it doesn't coalesce signals, it captures them all for assertions to be run later, assertions likeexpectRequest.
These tests are also used in the IdentityProcessorVerification tests which implements the SubscriberPuppet as forwarding the triggers as is to the processor under test. So, IIUC the invariants you state would have to be valid for any identity processor implementation you might want to test. That would mean, that you cannot use the TCK to check processor implementations that do coalescing of requests and cancel.
There was a problem hiding this comment.
In which case, perhaps we should change this to expectRequest before we signalCancel, and then insert a new expectCancelling after that, before we signalNext, to ensure that the subscriber did call cancel before we sent the next element - that's I think the original intent of the ordering of operations here, and matches the description of the test.
There was a problem hiding this comment.
I've implemented this here: #462
|
Nice work, @jroper! @reactive-streams/contributors Yay or nay? |
|
@reactive-streams/contributors Merging this without objections. |
|
Thanks @jroper! 👍 |
Fixes #277.
Fixes two whitebox subscriber tests where they weren't waiting for demand before signaling onNext.