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

Subscriber whitebox verification tests demand #280

Closed
jroper opened this issue Jun 30, 2015 · 8 comments

Comments

@jroper
Copy link
Contributor

commented Jun 30, 2015

So firstly, there's no javadocs on SubscriberPuppet. Is triggerRequest meant to trigger a request synchronously? Due to #277, it currently must be triggered synchronously, otherwise some of the tests violate the spec - but that's probably a problem with those tests.

Another thing not explained is whether exactly elements must be triggered. I suspect many subscribers (including the one I'm implementing, netty) do not allow this level of control over them, since the number of elements requested is controlled by buffers, and it's only possible for the test to signal an event to say it's possible to write more elements, not how many elements. Modifying the logic such that something external can trigger a specific number of elements would undermine the existing logic there, meaning that the TCK isn't testing the real subscriber.

So, as it happens, not requesting the exact number of elements still works, the TCK makes no assertions about how many elements were actually requested. But because of this, the TCK itself violates the spec, since it may end up sending more elements than were requested. Consider a streams implementation that only supports requesting one element at a time (such as iteratees), the TCK triggers a request for 2 elements, the iteratee requests one, and then the TCK sends 2 elements, a violation of the spec. It's only due to implementation details in each subscriber that this works.

So I'd like to propose that these tests be modified in the following way:

SubscriberPuppet.triggerRequest(long elements) be renamed to SubscriberPuppet.ensureDemand(long elements), and that the contract be documented to say that when this method is invoked, the subscriber must eventually request at least elements number of elements, but it may request those elements in batches of one or more, and is allowed to wait for the requested elements to be received by the onNext method before requesting the next batch. Then the TCK tests should be modified to, instead of just asserting that there was a request, read the request, and then send the number of elements that were requested, looping if necessary to read more requests if the full amount of demand wasn't requested.

@viktorklang

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2015

@jroper Why introduce a new method for this? Or am I missing something crucial?

@jroper

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2015

It's not introducing a new method, it's renaming a method to match its intent. The method may not necessarily trigger a request for that many elements, but it must ensure that there is eventually at least enough demand for that many elements.

@viktorklang

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2015

If we can rename it without breaking BC that's completely fine by me, as well as adding better docs for it.
I don't have enough spare cycles ATM so anyone, feel free to propose a PR.

@jroper

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2015

It's a Java interface, you can't add an abstract method without renaming.

But also note, renaming is not the most important thing here, the important thing is to fix the broken tests. For example:

  @Override @Test
  public void required_spec308_requestMustRegisterGivenNumberElementsToBeProduced() throws Throwable {
    subscriberTest(new TestStageTestRun() {
      @Override
      public void run(WhiteboxTestStage stage) throws InterruptedException {
        stage.puppet().triggerRequest(2);
        stage.probe.expectNext(stage.signalNext());
        stage.probe.expectNext(stage.signalNext());

It needs to wait until a request for at least one element has been made before signalling the first mesage, (this particular issue is already reported in #277), but it then needs to wait until it has requests for at least 2 elements (the first request may only have asked for one, because some subscribers only support requesting one element at a time) before sending the second.

@viktorklang

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2015

If we can change the method name without breaking backwards compat then I'm all for it. Otherwise a compromise would be to fix the semantics of the method and properly document that and that it has been changed.

Would you be able/willing to propose a PR to fix this?

@viktorklang

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

@jroper Ping

1 similar comment
@viktorklang

This comment has been minimized.

Copy link
Contributor

commented May 22, 2017

@jroper Ping

@viktorklang

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2017

Ping timeout

@viktorklang viktorklang closed this Dec 1, 2017

jroper added a commit to jroper/reactive-streams-jvm that referenced this issue Nov 19, 2018
Clarified the expected behaviour for triggerRequest
This addresses some of the problem raised in reactive-streams#280.
viktorklang added a commit that referenced this issue Dec 30, 2018
Clarified the expected behaviour for triggerRequest (#442)
This addresses some of the problem raised in #280.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.