PublisherVerification#required_spec317_mustNotSignalOnErrorWhenPendingAboveLongMaxValue takes a long time for fully synchronous publishers

[ldaley]

Hi,

I can't quite determine whether this is a defect in my test, or my interpretation of the spec.

I have an Iterable publisher, that is fully synchronous. With this test, it effectively tries to publish 0..Integer.MAX_VALUE which takes some time.

This won't return until all of the values have been published:

https://github.com/reactive-streams/reactive-streams-jvm/blob/master/tck/src/main/java/org/reactivestreams/tck/PublisherVerification.java#L1057

Solutions I can see:

1. Use something more reasonable than Integer.MAX_VALUE.
2. Cancel the subscription when callsCounter == 0

Or of course, my implementation is wrong.

[smaldini]

I think the intent is more about testing the pending request handling which can wrap if you request more long than long max (negative number) , which is going to fail somehow since negative demand is prohibited. The publisher helper should be passed an unlimited marker, at this stage you should not necessarily map the request to number of elements if it's Long.Max for the purpose of the test.

Sent from my iPhone

On 4 Apr 2015, at 1:14 am, Luke Daley notifications@github.com wrote:

Hi,

I can't quite determine whether this is a defect in my test, or my interpretation of the spec.

I have an Iterable publisher, that is fully synchronous. With this test, it effectively tries to publish 0..Integer.MAX_VALUE which takes some time.

This won't return until all of the values have been published:

https://github.com/reactive-streams/reactive-streams-jvm/blob/master/tck/src/main/java/org/reactivestreams/tck/PublisherVerification.java#L1057

Solutions I can see:

Use something more reasonable than Integer.MAX_VALUE.
Cancel the subscription when callsCounter == 0
Or of course, my implementation is wrong.

—
Reply to this email directly or view it on GitHub.

[ktoso]

Hi guys,
@alkemist you are right that a naive synchronous implementation will keep running forever here.
I think we can indeed improve this test for purely synchronous Publishers via your 2nd proposal, but I'd like to draw out the bigger picture about this test and how we handle it in Akka as well:

The purpose of this test is indeed to test the overflowing of demand (as @smaldini points out) - in order to check the Publisher doesn't onError or fail otherwise if demand overflows 3.17 - last sentence. We could use smaller numbers in each request and request more times... I don't feel like it's that is changing much though.

@alkemist I do agree that we can send the cancel() inside here and this might help some synchronous Publisher implementations (those that during their "loop sending onNext signals" check for cancellation). Do you want to send in the PR?

In Akka we initially also had the such naive implementation however later realised it's not the best we could do, and our interpreter should yield sometimes to respect cancellation in face of a downstream requesting Long.MAX_VALUE here's the issue: ActorInterpreter should yield to allow cancel even though there is demand #16983 (PR solving it). This also is to respect Rule 3.4: Subscription.request SHOULD respect the responsivity of its caller by returning in a timely manner[2]..

// Happy easter!

[ldaley]

Will send a PR in the next 48 hours.

[ktoso]

Great, thanks a lot @alkemist

[smaldini]

One solution in the meantime @alkemist is to override the spec test and @ignore it to not be blocked by that on RC4 update.

[viktorklang]

Looks like we ought to issue an RC5 with the fix ASAP.
Given the number of already passing impls for RC4 we ought to be ready to cut 1.0.0.final after RC5 is confirmed to fix the aforementioned issue and Ratpack passes the TCK?

[viktorklang]

Merged