Spec. 2. 12 TCK : Testing a unicast publisher

[smaldini]

Hi there,
After updating to the latest 1.0.0.RC1 (#84), Reactor had an issue with spec 2.12 that can be easily fixed but that asks for a quick confirmation/question.

Spec 2.12 forbids double subscription of a given Subscriber to a same Publisher. However we acknowledged there are two kinds of publisher, cold and hot streams. In the cold stream case it seems enough and more efficient to not maintain a registry of subscriptions because each subscription can usually track alone their state: e.g. an Iterable Publisher maintains a single reference to the iterable and a cursor by subscription to track the requests. In our test that is the case where we publish from a helper IterableStream (Streams.from([1,2,3,...]).

However the 2.12 test fails since the two subscriber are actually orthogonal, they start iterating onNext from index 0 and thats pretty much it. To force a shared registry, we offer a Stream#broadcast operation that transforms the cold iterable stream into a hot stream and therefore maintains an active registry of subscriptions (fulfilling the 2.12 contract) that will receive elements T when they are actually subscribed, whatever the cursor upstream from the iterable publisher is actually incremented to.

So if we validate the assumption there are 2 types of streams, should that test mandate use of an hot stream (and therefore requires an hot stream publisher factory method specifically for that test ?). The test should actually validate the cold stream behavior too in fact. That would also need a quick amend to the spec to mention these two specific behaviors: exclusive on hot stream and inclusive on cold streams.
Another way to look at this is to mandate that the subscriber does actually support only 1 subscription and will assume the ownership of the relation with the publisher, that would actually move the current test to the subscriber verification instead of publisher.
Thoughts ?

[viktorklang]

Hi @smaldini,
I think this ties into the discussion here: #178

I'll check the actual TCK verification to check if it makes sense, but the subscriber should cancel subsequent subscriptions according to 2.5.

[viktorklang]

On inspection, I assume you are using using the whitebox verification for subscriber:
https://github.com/reactive-streams/reactive-streams/blob/master/tck/src/main/java/org/reactivestreams/tck/SubscriberWhiteboxVerification.java#L347

Whereas the blackbox verification does the following: https://github.com/reactive-streams/reactive-streams/blob/master/tck/src/main/java/org/reactivestreams/tck/SubscriberBlackboxVerification.java#L285

Since we're talking about 2.12 which is a Subscriber rule, it is more about the -usage- of the Subscriber rather than the implementation thereof, since the appropriate action is governed by 2.5. The problem is that since Subscribers are not guaranteed to be safe for concurrent access, 2.5 cannot be tested 100%, so 2.12 is advise to the user.

So the lowest friction course of action I suspect is to drop the verification of 2.12 and see it as a complimentary rule to 1.10 which is about how to use Publisher/Subscriber.

Thoughts @reactive-streams/contributors?
Thoughts @ktoso?

[viktorklang]

To me it seems worth while to incorporate this fix + #186 for RC2.

[ktoso]

whitebox 2.12 is supposed to be an Additional test currently already - @smaldini are you saying that instead of becoming a SKIP it actually FAILs? If yes, than that's a problem we should fix anyway.

I'd be fine with either removing this verification completely or keeping it around but making sure it does act like an additional one properly (when test fails, become a skipped test, not a failure).

// Note: Didn't dig deep into this issue as I'm packing up for a week-trip right now. I'll be back around the 7th.

[viktorklang]

Making it Additional makes sense to me, but then Blackbox 2.12 should be
the same (instead of notVerified)?

On Sun, Dec 28, 2014 at 8:28 PM, Konrad Malawski [email protected]
wrote:

whitebox 2.12 is supposed to be an Additional test currently already -
@smaldini https://github.com/smaldini are you saying that instead of
becoming a SKIP it actually FAILs? If yes, than that's a problem we should
fix anyway.

I'd be fine with either removing this verification completely or keeping
it around but making sure it does act like an additional one properly (when
test fails, become a skipped test, not a failure).

// Note: Didn't dig deep into this issue as I'm packing up for a week-trip
right now.

—
Reply to this email directly or view it on GitHub
#188 (comment)
.

Cheers,
√

[smaldini]

I was using IdentityTestProcessor in that test and it doesn't actually skip it it seems.

[viktorklang]

Looking at https://github.com/reactive-streams/reactive-streams/blob/master/tck/src/main/java/org/reactivestreams/tck/IdentityProcessorVerification.java#L543 I suspect when overriding one must also re-annotate the @Required/@Additional, I think the proper course of action is to open a ticket to go through the TCK and to place @Required/@Additional on all the overrides.

[viktorklang]

@smaldini Do you have a chance to try that suggestion and see if that works?

[viktorklang]

@smaldini could you try #189?

[viktorklang]

Btw, do you have a stacktrace of your failure?

[viktorklang]

@smaldini After having gone through the TCK and the spec, I think 2.12 should be a nonverified spec rule as there is no way to deterministically test it.
I am updating the TCK to rectify this in my PR, thanks a lot for finding this!

[smaldini]

Makes sense, should that call for RC2 ?

On Mon, Dec 29, 2014 at 9:54 PM, Viktor Klang (√) [email protected]
wrote:

@smaldini https://github.com/smaldini After having gone through the TCK
and the spec, I think 2.12 should be a nonverified spec rule as there is no
way to deterministically test it.
I am updating the TCK to rectify this in my PR, thanks a lot for finding
this!

—
Reply to this email directly or view it on GitHub
#188 (comment)
.

Stéphane

[viktorklang]

I'd say so, but I'd like to see if we can include Konrads TCK improvements
too. I'll try to review his PR tonight, feel free/encouraged to do the same
if you have time :)

Cheers,
√
On 30 Dec 2014 12:20, "Stephane Maldini" [email protected] wrote:

Makes sense, should that call for RC2 ?

On Mon, Dec 29, 2014 at 9:54 PM, Viktor Klang (√) <
[email protected]>
wrote:

@smaldini https://github.com/smaldini After having gone through the
TCK
and the spec, I think 2.12 should be a nonverified spec rule as there is
no
way to deterministically test it.
I am updating the TCK to rectify this in my PR, thanks a lot for finding
this!

—
Reply to this email directly or view it on GitHub
<
https://github.com/reactive-streams/reactive-streams/issues/188#issuecomment-68307447>

.

Stéphane

—
Reply to this email directly or view it on GitHub
#188 (comment)
.