Skip to content

fix contradicting provisions around subscribing multiple times #63

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

Closed
rkuhn opened this issue Jun 14, 2014 · 26 comments
Closed

fix contradicting provisions around subscribing multiple times #63

rkuhn opened this issue Jun 14, 2014 · 26 comments

Comments

@rkuhn
Copy link
Member

rkuhn commented Jun 14, 2014

We need to find wording which mandates sane behavior while not requiring all Publishers to keep track of all the past Subscribers indefinitely.

@benjchristensen
Copy link
Contributor

#61 (comment)

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

@benjchristensen
Copy link
Contributor

#61 (comment)

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

@benjchristensen
Copy link
Contributor

#61 (comment)

+| 13 | While the Subscription is not cancelled, Subscription.cancel() MUST request the Publisher to eventually drop any references to the corresponding subscriber. Re-subscribing with the same Subscriber object is discouraged [see 2.12], but this specification does not mandate that it is disallowed since that would mean having to store previously canceled subscriptions indefinitely |

@smaldini
Copy link
Contributor

Is the subscribe operation required to be thread safe ? I'd like to be adaptive and provide a composite subscription only for multicast (more than 1 successful subscribe call). Problem occurs when switching Publisher reference to subscription (to broadcast hot events), for a composite subscription. This operation needs a memory barrier to access the subscription field and update it. I'd just like to avoid that by assuming external synchronization if subscribe is called by different threads/resources.

@viktorklang
Copy link
Contributor

@smaldini Could you elaborate on the problem? I assume you control the Publisher implementation?

@viktorklang
Copy link
Contributor

@smaldini ping

@viktorklang
Copy link
Contributor

Thinking more about this I assume that subscribe needs to be thread safe, or rather, safe for concurrent subscribers.

@smaldini
Copy link
Contributor

smaldini commented Jul 9, 2014

This could hurt performances tho, I mean the implementor can make sure of this but enforcing it is a serious constraint to consider. But if we optimise for unicasting, dynamically switching to multi casting (with a multi subscriptions Subscription).

Sent from my iPhone

On 9 Jul 2014, at 14:58, Viktor Klang (√) [email protected] wrote:

Thinking more about this I assume that subscribe needs to be thread safe, or rather, safe for concurrent subscribers.


Reply to this email directly or view it on GitHub.

@viktorklang
Copy link
Contributor

How many subscribers per second are you anticipating?
On Jul 9, 2014 4:05 PM, "Stephane Maldini" [email protected] wrote:

This could hurt performances tho, I mean the implementor can make sure of
this but enforcing it is a serious constraint to consider. But if we
optimise for unicasting, dynamically switching to multi casting (with a
multi subscriptions Subscription).

Sent from my iPhone

On 9 Jul 2014, at 14:58, Viktor Klang (√) [email protected]
wrote:

Thinking more about this I assume that subscribe needs to be thread
safe, or rather, safe for concurrent subscribers.


Reply to this email directly or view it on GitHub.


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

@smaldini
Copy link
Contributor

smaldini commented Jul 9, 2014

The issue is more about the stream data rate. It can only be 1 subscriber with 10M onNext / s. Introducing the subscriber in this stream will block/add overhead, but there might be solutions to that I need investigate anyway. I would be reluctant technically speaking but the rule would make sense in general.

Sent from my iPhone

On 9 Jul 2014, at 15:13, Viktor Klang (√) [email protected] wrote:

How many subscribers per second are you anticipating?
On Jul 9, 2014 4:05 PM, "Stephane Maldini" [email protected] wrote:

This could hurt performances tho, I mean the implementor can make sure of
this but enforcing it is a serious constraint to consider. But if we
optimise for unicasting, dynamically switching to multi casting (with a
multi subscriptions Subscription).

Sent from my iPhone

On 9 Jul 2014, at 14:58, Viktor Klang (√) [email protected]
wrote:

Thinking more about this I assume that subscribe needs to be thread
safe, or rather, safe for concurrent subscribers.


Reply to this email directly or view it on GitHub.


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


Reply to this email directly or view it on GitHub.

@viktorklang
Copy link
Contributor

How (what Thread) would new Subscribers normally enter the picture (given
that the Subscriber does not know the Publisher)?
If you have a hot stream and you want to dynamically add Subscribers you
have to coordinate with the production.
On Jul 9, 2014 4:18 PM, "Stephane Maldini" [email protected] wrote:

The issue is more about the stream data rate. It can only be 1 subscriber
with 10M onNext / s. Introducing the subscriber in this stream will
block/add overhead, but there might be solutions to that I need investigate
anyway. I would be reluctant technically speaking but the rule would make
sense in general.

Sent from my iPhone

On 9 Jul 2014, at 15:13, Viktor Klang (√) [email protected]
wrote:

How many subscribers per second are you anticipating?
On Jul 9, 2014 4:05 PM, "Stephane Maldini" [email protected]
wrote:

This could hurt performances tho, I mean the implementor can make sure
of
this but enforcing it is a serious constraint to consider. But if we
optimise for unicasting, dynamically switching to multi casting (with
a
multi subscriptions Subscription).

Sent from my iPhone

On 9 Jul 2014, at 14:58, Viktor Klang (√) [email protected]

wrote:

Thinking more about this I assume that subscribe needs to be thread
safe, or rather, safe for concurrent subscribers.


Reply to this email directly or view it on GitHub.


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

.


Reply to this email directly or view it on GitHub.


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

@benjchristensen
Copy link
Contributor

Thinking more about this I assume that subscribe needs to be thread safe, or rather, safe for concurrent subscribers.

This seems like it should be up to each implementation to worry about, doesn't it? A unicast/cold source starts fresh each time, whereas a multicast/hot source is sharing state. The latter would handle concurrency as needed whereas the former wouldn't need to as it shares no state.

@benjchristensen
Copy link
Contributor

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

I don't even understand this rule. Why would a Subscriber be receiving multiple Subscriptions from multiple Publishers? I say replace this with something like:

  • A Subscriber instance MUST only ever be subscribed to a single Publisher.

Rule 1.11 also gets involved in this.

1.11 It is RECOMMENDED to reject the Subscription with a java.lang.IllegalStateException if the same Subscriber already has an active Subscription with this Publisher.

I suggest this line needs to be removed and just be covered by the replacement I suggest above about a Subscriber only being subscribed to a single Publisher. The word RECOMMENDED is fairly strong still but makes no sense on unicast Publishers. I think it should be the responsibility of the Subscriber and not the Publisher to only subscribe to one thing at a time.

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

Line 2.12 prevents a Subscriber from being reused. Is that the intent?

Or are we instead trying to say that for any given Publisher.subscribe invocation the Subscriber passed in should only have onSubscribe called at most once (and possibly not at all if onError is called)?

Either way, this is where the contradiction with 3.13 happens:

3.13 While the Subscription is not cancelled, Subscription.cancel() MUST request the Publisher to eventually drop any references to the corresponding subscriber. Re-subscribing with the same Subscriber object is discouraged [see 2.12], but this specification does not mandate that it is disallowed since that would mean having to store previously canceled subscriptions indefinitely

Note this portion "Re-subscribing with the same Subscriber object is discouraged [see 2.12]," where we say it is discouraged, yet in 2.12 we say MUST NOT so actually prevent re-use as currently worded.

Possibly 2.12 could be replaced with:

  • A Publisher MUST NOT call Subscriber.onSubscribe more than once in a subscribe lifecycle.

Thus, I'd do the following:

  • delete current 2.5
  • modify or remove 2.12
  • leave 3.13 as is ... or eliminate the last sentence
  • remove second half of 1.11
  • add "A Subscriber instance MUST only ever be subscribed to a single Publisher."
  • add "A Publisher MUST NOT call Subscriber.onSubscribe more than once in a subscribe lifecycle."

@viktorklang
Copy link
Contributor

On Fri, Jul 18, 2014 at 6:27 PM, Ben Christensen [email protected]
wrote:

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

I don't even understand this rule. Why would a Subscriber be receiving
multiple Subscriptions from multiple Publishers? I say replace this with
something like:

  • A Subscriber instance MUST only ever be subscribed to a single
    Publisher.

So this mean that even if a Subscriber doesn't have an active subscription,
but it once had, it cannot be reused. Is this intentional?

Rule 1.11 also gets involved in this.

1.11 It is RECOMMENDED to reject the Subscription with a
java.lang.IllegalStateException if the same Subscriber already has an
active Subscription with this Publisher.

I suggest this line needs to be removed and just be covered by the
replacement I suggest above about a Subscriber only being subscribed to a
single Publisher. The word RECOMMENDED is fairly strong still but makes
no sense on unicast Publishers. I think it should be the responsibility of
the Subscriber and not the Publisher to only subscribe to one thing at a
time.

Subscribers "never" subscribe though, it's almost always outside code that
links the publisher with the subscriber via the subscribe call.

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

Line 2.12 prevents a Subscriber from being reused. Is that the intent?

Good question, should it?

Or are we instead trying to say that for any given Publisher.subscribe
invocation the Subscriber passed in should only have onSubscribe called
at most once (and possibly not at all if onError is called)?

This would mean that you couldn't cache Subscriber instances and reuse
them, for instance:

val devnull = new Subscriber[Any] {
  override def onSubscribe(s: Subscription) = s.request(Long.MaxValue)
  override def onNext(element: Any) = ()
  override def onComplete() = ()
  override def onError(t: Throwable) = ()
}

But that may be a good thing to disallow, I'm just raising it here so that
we can have it logged in case someone wants to know the history of the
decisions.

Either way, this is where the contradiction with 3.13 happens:

3.13 While the Subscription is not cancelled, Subscription.cancel() MUST
request the Publisher to eventually drop any references to the
corresponding subscriber. Re-subscribing with the same Subscriber object is
discouraged [see 2.12], but this specification does not mandate that it is
disallowed since that would mean having to store previously canceled
subscriptions indefinitely

Note this portion "Re-subscribing with the same Subscriber object is
discouraged [see 2.12]," where we say it is discouraged, yet in 2.12 we say
MUST NOT so actually prevent re-use as currently worded.

Possibly 2.12 could be replaced with:

  • A Publisher MUST NOT call Subscriber.onSubscribe more than once in a
    subscribe lifecycle.

Doesn't that imply that a Publisher must remember all active subscribers?
(I vaguely recall you objecting to that in earlier discussions)

Thus, I'd do the following:

  • delete current 2.5
  • modify or remove 2.12
  • leave 3.13 as is ... or eliminate the last sentence
  • remove second half of 1.11
  • add "A Subscriber instance MUST only ever be subscribed to a single
    Publisher."
  • add "A Publisher MUST NOT call Subscriber.onSubscribe more than once
    in a subscribe lifecycle."

Before we do anything, perhaps we should propose semantics for
subscriber+onSubscribe and then we can derive what actions that are needed
from that?


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

Cheers,

@danarmak
Copy link

Re-subscribing a Subscriber to a different Publisher after the current Publisher calls onComplete is useful. For instance, when concatenating Publishers, especially where the list of all Publishers is not known in advance. I occasionally have to feed data from multiple sources into the same (stateful) processor, one after the other.

If you forbid this, then in such situations one will have to create a Subscriber for each Publisher that simply passes data to a the same (stateful) object, let's call it a collector, whose only significance is that it looks just like a Subscriber but doesn't actually implement the Subscriber interface, so it's allowed to be resubscribed. That's not a big burden technologically (at least if subscribe events are infrequent and there is no performance problem), it just looks ugly IMO.

On the other hand, an interleave or merge combinator that subscribes to multiple Publishers at once is already forbidden from using the same Subscriber, and so has to use pretty much this technique. So maybe from the point of view of someone who does merge more often than concat (unlike me), this is just a generalization of an existing principle and we might as well do it.

@viktorklang
Copy link
Contributor

On Jul 18, 2014 11:43 PM, "√iktor Ҡlang" [email protected] wrote:

On Fri, Jul 18, 2014 at 6:27 PM, Ben Christensen [email protected]
wrote:

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

I don't even understand this rule. Why would a Subscriber be receiving
multiple Subscriptions from multiple Publishers? I say replace this with
something like:

A Subscriber instance MUST only ever be subscribed to a single Publisher.

So this mean that even if a Subscriber doesn't have an active
subscription, but it once had, it cannot be reused. Is this intentional?

Rule 1.11 also gets involved in this.

1.11 It is RECOMMENDED to reject the Subscription with a
java.lang.IllegalStateException if the same Subscriber already has an
active Subscription with this Publisher.

I suggest this line needs to be removed and just be covered by the
replacement I suggest above about a Subscriber only being subscribed to a
single Publisher. The word RECOMMENDED is fairly strong still but makes no
sense on unicast Publishers. I think it should be the responsibility of the
Subscriber and not the Publisher to only subscribe to one thing at a time.

Subscribers "never" subscribe though, it's almost always outside code
that links the publisher with the subscriber via the subscribe call.

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

Line 2.12 prevents a Subscriber from being reused. Is that the intent?

Good question, should it?

Or are we instead trying to say that for any given Publisher.subscribe
invocation the Subscriber passed in should only have onSubscribe called at
most once (and possibly not at all if onError is called)?

This would mean that you couldn't cache Subscriber instances and reuse
them, for instance:

 val devnull = new Subscriber[Any] {
   override def onSubscribe(s: Subscription) = s.request(Long.MaxValue)
   override def onNext(element: Any) = ()
   override def onComplete() = ()
   override def onError(t: Throwable) = ()
 }

But that may be a good thing to disallow, I'm just raising it here so
that we can have it logged in case someone wants to know the history of the
decisions.

Either way, this is where the contradiction with 3.13 happens:

3.13 While the Subscription is not cancelled, Subscription.cancel()
MUST request the Publisher to eventually drop any references to the
corresponding subscriber. Re-subscribing with the same Subscriber object is
discouraged [see 2.12], but this specification does not mandate that it is
disallowed since that would mean having to store previously canceled
subscriptions indefinitely

Note this portion "Re-subscribing with the same Subscriber object is
discouraged [see 2.12]," where we say it is discouraged, yet in 2.12 we say
MUST NOT so actually prevent re-use as currently worded.

Possibly 2.12 could be replaced with:

A Publisher MUST NOT call Subscriber.onSubscribe more than once in a
subscribe lifecycle.

Doesn't that imply that a Publisher must remember all active subscribers?
(I vaguely recall you objecting to that in earlier discussions)

Thus, I'd do the following:

delete current 2.5
modify or remove 2.12
leave 3.13 as is ... or eliminate the last sentence
remove second half of 1.11
add "A Subscriber instance MUST only ever be subscribed to a single
Publisher."
add "A Publisher MUST NOT call Subscriber.onSubscribe more than once in
a subscribe lifecycle."

Before we do anything, perhaps we should propose semantics for
subscriber+onSubscribe and then we can derive what actions that are needed
from that?

Ack, typo: I meant semantics of Publisher.subscribe and
Subscriber.onSubscribe.


Reply to this email directly or view it on GitHub.

Cheers,

@viktorklang
Copy link
Contributor

@reactive-streams/contributors Ping.

@viktorklang
Copy link
Contributor

@reactive-streams/contributors Pong.

@viktorklang
Copy link
Contributor

@reactive-streams/contributors Ping.

@viktorklang
Copy link
Contributor

@reactive-streams/contributors Vote to close due to inactivity and failure to observe proper Ping/Pong protocol. 👍 or 👎?

@benjchristensen
Copy link
Contributor

The contradictions still exist. Should I submit a pull request with my suggestions (#63 (comment))?

@viktorklang
Copy link
Contributor

Submit PR so we can discuss and settle it before 1.0.0-RC1 :-)
On Oct 24, 2014 7:26 PM, "Ben Christensen" [email protected] wrote:

The contradictions still exist. Should I submit a pull request with my
suggestions (#63 (comment)
#63 (comment)
)?


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

@viktorklang
Copy link
Contributor

@benjchristensen ping

@benjchristensen
Copy link
Contributor

I'm crushed right now so it's not going to happen for me until after ReactConf. This stuff is good enough for 1.0-RC1. I propose leaving this open for me to attempt clarifying things later.

@viktorklang
Copy link
Contributor

@benjchristensen No worries. Let's revisit when possible. Your work on the 1.0 release track is inspiring to say the least!

@viktorklang
Copy link
Contributor

Moving this discussion to #178 :-) (reopen if you feel this was not appropriate)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants