Simplify 1.10 and clarify 2.12

[viktorklang]

I propose to remove the following section from 1:10:

"It is RECOMMENDED to reject the Subscription with a java.lang.IllegalStateException if the same Subscriber already has an active Subscription with this Publisher. The cause message MUST include a reference to this rule and/or quote the full rule"

Rationale: As @benjchristensen has pointed out to me several times, and I've been too thick to get (apparently), it doesn't make sense to require the Publisher to keep track of this, and it does not prevent concurrent signalling anyway since the same Subscriber could be passed into multiple different Publishers without being prevented by this part of the rule.

At the same time, I'd propose to clarify the Subscriber's responsibility here (2.12) by clarifying 2.12 from:

"Subscriber.onSubscribe MUST NOT be called more than once (based on object equality)"

to

"A Subscriber that has already received a Subscription via onSubscribe MUST cancel any further Subscriptions it receives via onSubscribe."

Rationale: This means that the Subscriber is the "source of truth" if there is a risk of multiple subscriptions. The strategy to deal with the situation is harmless, as the Subscriber can cancel it immediately. (as well as log the error condition)

@reactive-streams/contributors What do you think about this? I think it offers significant simplification.

[viktorklang]

Note that this does not really change any semantics, the RECOMMENDED part is still legal, spec-wise, but doesn't burden the spec anymore, and the clarification of 2.12 is just healthy given that it is arguably weirdly worded right now.

[rkuhn]

The first part makes sense to me. In order to make the second part consistent with it, I would rather propose the following replacement for 2:12:

A Subscriber MUST NOT be attached to more than one Publisher, and it MUST NOT be attached to the same Publisher more than once. The Publisher it is attached to MUST NOT call onSubscribe more than once in response to a subscribe invocation.

This makes it clear that a Subscriber is intended as a single-use object without placing restrictions on Publisher implementations beyond just not calling onSubscribe twice in response to subscribe.

[viktorklang]

@rkuhn The problem is that it is unenforcable from the Publisher-side of things, one Publisher will not know that another Publisher has already called onSubscribe on the same Subscriber. Therefor I think it needs to fall onto the Subscriber to cancel subsequent Subscriptions. Or is my logic faulty?

[benjchristensen]

👍 on both your suggestions @viktorklang

I agree with @viktorklang that it is up to the Subscriber to do the right thing.

[rkuhn]

@viktorklang Your logic is flawless, so let’s make the text match up with it :-) This means that 2.12 needs to be changed to:

• A Subscriber that has already received a Subscription (via onSubscribe) MUST cancel any further Subscriptions it receives.

[benjchristensen]

Curious, is there a reason to not just say "A Subscriber MUST only be subscribed to 1 Publisher"?

[rkuhn]

That is indeed what I would like it to mean, but unfortunately this is problematic: who shall enforce this and how? This issue is exemplified by the fact that this cannot be tested, in contrast to my proposal which is quite simple to test.

[viktorklang]

@rkuhn Thanks, I like your rewording to be a positive wording instead of my MUST NOT.

[viktorklang]

@reactive-streams/contributors Updated proposal after discussion.

[viktorklang]

@rkuhn & @benjchristensen, there is one issue though, that even if the Subscriber can cancel subsequent subscriptions, it can still get an onComplete or onError from the additional Publishers. Thoughts?

[rkuhn]

This is why the intention must be documented: that case is a user error, and it is one that implementations cannot fully defend against.

[viktorklang]

@rkuhn That is a good point, these "rationale"/"intent" notes seems like a good idea.

[benjchristensen]

I don't think this is something we can deterministically assert via TCK because we don't control the Subscriber implementation.

Just like concurrency is not something easily asserted, I don't see a guaranteed way of asserting via tests that an implementation of a Subscriber is NOT subscribing to multiple publishers at the same time.

I think that's okay.

That is why I prefer the simpler "A Subscriber MUST only be subscribed to 1 Publisher". How they achieve this is up to them. The simplest is "just don't subscribe more than once". Then they won't have to do the awkward "unsubscribe immediately" thing which doesn't make sense if they just never subscribe more than once to begin with.

[viktorklang]

I don't think this is something we can deterministically assert via TCK because we don't control the Subscriber implementation.

I concur, since Subscriber isn't mandated to be thread safe for the synchronous implementation.

Just like concurrency is not something easily asserted, I don't see a guaranteed way of asserting via tests that an implementation of a Subscriber is NOT subscribing to multiple publishers at the same time.
That is why I prefer the simpler "A Subscriber MUST only be subscribed to 1 Publisher". How they achieve this is up to them. The simplest is "just don't subscribe more than once". Then they won't have to do the awkward "unsubscribe immediately" thing which doesn't make sense if they just never subscribe more than once to begin with.

True. One issue though—the person writing the Subscriber is most likely not the person who puts it into a Publisher.subscribe. And, the question is what happens if it is violated. (If different implementations do different things, the risk is that users of one implementation gets used to doing it wrong, because nothing bad usually happens with one implementation, but throw another into the mix and now other weird stuff starts to happen.

I like your simpler form, and added a small tweak to it: A Subscriber MUST only be subscribed to a single Publisher and only once.

To ensure consistency in the error case, we can add the same clause as for 2:13:

"In the case that this rule is violated, any associated Subscription to the Subscriber MUST be considered as cancelled, and the invoker MUST raise this error condition in a fashion that is adequate for the runtime environment"

I thought a bit about it, and I do think it makes sense to cancel -any associated Subscription- (i.e. all of them), since this makes race conditions less likely (i.e. avoid first-commit-wins).

@benjchristensen wdyt? @rkuhn?

[rkuhn]

I agree, with one further tweak: replace “considered as cancelled” with “cancelled”, because otherwise there might be Publishers that have no clue that something went wrong.

[viktorklang]

Nice catch

On Wed, Dec 17, 2014 at 9:04 PM, Roland Kuhn [email protected]
wrote:

I agree, with one further tweak: replace “considered as cancelled” with
“cancelled”, because otherwise there might be Publishers that have no clue
that something went wrong.

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

Cheers,
√

[viktorklang]

After some thinking I will only propose to change 1:10 to drop the RECOMMENDED section as suggested by @benjchristensen, since 2.5 already does what was proposed in this Issue for 2.10:

"A Subscriber MUST call Subscription.cancel() on the given Subscription after an onSubscribe signal if it already has an active Subscription"