Question on validity of SubscriberBlackboxVerification.required_spec201_blackbox_mustSignalDemandViaSubscriptionRequest

[purplefox]

Hi folks,

This test is currently failing when run against Vert.x.

The test appears to expect Subscription.request(..) to be called sometime shortly after the onSubscribe method of the Subscriber is called.

From the docs, it appears this is trying to validate Subscriber requirement 2.1:

https://github.com/reactive-streams/reactive-streams-jvm#2.1

"A Subscriber MUST signal demand via Subscription.request(long n) to receive onNext signals."

However, I can't see anything in the above that says request() must be invoked when the onSubscribe method is called.

The Vert.x subscriber implementation won't call Subscription.request until some time later when the user has actually set a handler on the subscriber.

So I would like to question the validity of this test.

[ktoso]

We never force anyone to synchronously call things, in fact in Akka all these things are asynchronous as well, so I don't think this is the problem here.

This test contains:

        final long n = stage.expectRequest();// assuming subscriber wants to consume elements...

the expectRequest uses a timeout, more specifically - the timeout you provide to TestEnvironment as constructor argument. The error message you got probably when this test failed should have included the timeout as well, so you can adjust it to what suits your impls.

Unless you're stating the TCK can't require people to request()... but then I mean, it can't do anything without this.. :-)

Sources:

• reactive-streams-jvm/tck/src/main/java/org/reactivestreams/tck/TestEnvironment.java

  Line 662 in b33420a

  return expectRequest(env.defaultTimeoutMillis());

• reactive-streams-jvm/tck/src/main/java/org/reactivestreams/tck/SubscriberBlackboxVerification.java

  Line 77 in b33420a

  final long n = stage.expectRequest();// assuming subscriber wants to consume elements...

[purplefox]

I'm not claiming the test requires subscription.request() to be called synchronously after onSubscribe is called.
But it does require it to be called sometime afterwards, without any other interactions going on. This does not appear to be a valid requirement - at least I can't see anywhere in the spec that says "onSubscribe should cause subscription.requestMore" to be called, but I could be missing something.

[purplefox]

Let me explain this with some more detail...

The Vert.x Subscriber implementation is called ReactiveReadStream:
https://github.com/vert-x3/vertx-reactive-streams/blob/master/src/main/java/io/vertx/ext/reactivestreams/ReactiveReadStream.java

It would be used something like this:

ReactiveReadStream rrs = ReactiveReadStream.create();
MyPublisher publisher = new MyPublisher();
publisher.subscribe(rrs); // Position A
rrs.handler(buffer -> System.out.println("Got some data")); // Position B

The TCK seems to assume that requestMore is called at position A in the above code, whereas with Vert.x it's called at position B.

It seems to be that it is implementation dependent exactly when requestMore gets called, but the TCK is assuming otherwise.

[purplefox]

On the basis that when requestMore gets called is implementation dependent, then one way of fixing the TCK test would be to provide a new method on SubscriberBlackboxVerification which can be implemented by me which calls the implementation specific method that ensures the initial requestMore gets called, e.g. I would implement this on my class that extends SubscriberBlackboxVerification:

@OverRide
public void callInitialRequestMore() {
myimplementation.handler(buff -> });
}

[drewhk]

This seems reasonable, the spec does not require when to call request().

[purplefox]

(BTW, sorry about the crappy formatting in the above comments)

[ktoso]

Thanks for the code example, this explained a bit.
Seems like we indeed might have to provide such hook - I'll give it a shot today in the evening.

Thanks for the excellent feedback; It's very interesting how differently each impl has tackled the smaller nuances of the spec :-)

[purplefox]

No problem! Thanks for helping me with this :)

[viktorklang]

@ktoso what's the status here?

[ktoso]

Need to re-attack this one, hoping to PR over the weekend.

[ktoso]

Been trying to get to it, but under some pressure in other project.
Hoping to get to it soon, it's important and non-trivial (tried once and failed, was tired though), I consider this a must for 1.0, so Tim's impl doesn't need to revert to hacks to pass the TCK.

[viktorklang]

@ktoso Let me know if you want me to pull some strings.

[ktoso]

Would be very cool actually, I'm a bit overloaded right now. Mind giving this one a shot?

[viktorklang]

@ktoso It'll be easier for me to make you less overloaded than to figure out what is left to be fixed here. :)

[rkuhn]

We’ll have to defer this until next Monday unless someone else wants to pick it up—sorry for the NoResourcesRightNowException.

[viktorklang]

@rkuhn @ktoso If it is possible to do a quick writeup what's left to do and how to verify, I can try to allocate more resources with my OCTO-allocator. So we can ship the next RC this week.

[ktoso]

As much as I'd like to work on this now, there's not enough hours in a day so that I could squeeze it in this week, given other project involvements. Not sure if it would be merged in time for this RC if implemented today btw?

Quick summary is:

• some impls do not trigger the initial request() call when they get the subscription, they do it after some external signal triggers them
• we must provide a method called triggerRequest(Subscriber) (the subscriber under test, passing it into the method is preferred as userland does not need a var for it then) in both SubscriberVerifications
• it must have an empty body by default, users should be able to override it such that they can trigger execution, like for example subscriber.seriouslyRun()
• this needs to be called whenever we want the subscriber to do requests. I'm assuming this means after handing out a subscription, and when a whitebox probe tries to force requesting (from memory, may be mistaken on whitebox).

@purplefox is using a workaround currently, as seen here:

• https://github.com/vert-x3/vertx-reactive-streams/blob/master/src/main/java/io/vertx/ext/reactivestreams/impl/ReactiveReadStreamImpl.java#L36
• https://github.com/vert-x3/vertx-reactive-streams/blob/e5d73c8f011f9bd2dd7c98515f39cb0109f41971/src/test/java/io/vertx/ext/reactivestreams/tck/SubscriberBlackboxVerificationTest.java

Goal of this change is to not force Tim to use this hack.

[viktorklang]

@ktoso No need to apologize, I fully understand the predicament.

I'll see if I can free some time to work on this so we can get 1.0.0 out the door sooner.
Of course, any of the @reactive-streams/contributors are encouraged to help out!

[viktorklang]

A good way of implementing this would be to create a simple AsyncSubscriber impl (based off / extending the examples one) and have it request from upstream only by external event, then implement the Blackbox and Whitebox verification and fix all tests that fail by delegating to a method that allows the implementer to call their trigger.

Unless someone else gets to it before me I'll try to get this done either this evening or tomorrow.

[viktorklang]

In case anybody decides to pick this up until I have some more minutes available, here's my WIP-branch which has a commit now with a reproducer that fails the build: https://github.com/reactive-streams/reactive-streams-jvm/tree/wip-233-support-manual-demand-%E2%88%9A

[viktorklang]

Did I get it right?

#252