=tck #362 signal onComplete in 201 blackbox verification #372
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
ktoso
commented
May 29, 2017
| @@ -96,11 +96,16 @@ public void required_spec201_blackbox_mustSignalDemandViaSubscriptionRequest() t | |||
| @Override | |||
| public void run(BlackboxTestStage stage) throws InterruptedException { | |||
| triggerRequest(stage.subProxy().sub()); | |||
| final long n = stage.expectRequest();// assuming subscriber wants to consume elements... | |||
| final long requested = stage.expectRequest();// assuming subscriber wants to consume elements... | |||
| final long signalsToEmit = Math.min(requested, 512); // protecting against Subscriber which sends ridiculous large demand | |||
There was a problem hiding this comment.
Or could be TestEnvironment.TEST_BUFFER_SIZE, anything less than huge numbers is fine here though IMO
akarnokd
reviewed
May 29, 2017
|
|
||
| // should cope with up to requested number of elements | ||
| for (int i = 0; i < n; i++) | ||
| for (int i = 0; i < signalsToEmit; i++) |
There was a problem hiding this comment.
This could also check if the Subscriber signalled cancel and not loop unnecessarily.
akarnokd
approved these changes
May 29, 2017
viktorklang
reviewed
May 29, 2017
|
|
||
| // we complete after `signalsToEmit` (which can be less than `requested`), | ||
| // which is legal under https://github.com/reactive-streams/reactive-streams-jvm#1.2 | ||
| stage.sendCompletion(); |
There was a problem hiding this comment.
Should we sendCompletion() even if cancelled?
There was a problem hiding this comment.
Why not? Cancellation is allowed to be "racy"
There was a problem hiding this comment.
I guess the "polite" implementation would check if it is not cancelled before issuing the completion. (so that the implementation does not rely on onComplete if cancelled)
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.
Follow up to things found in #362