Description
Considering a simple example:
public class UnsafeSubscriber implements Subscriber<String> {
private boolean duplicateOnSubscribe = false;
@Override
public void onSubscribe(final Subscription s) {
if (duplicateOnSubscribe) {
throw new IllegalStateException("Duplicate onSubscribe() calls.");
}
duplicateOnSubscribe = true;
}
@Override
public void onNext(final String s) {
}
@Override
public void onError(final Throwable t) {
}
@Override
public void onComplete() {
}
}
If an UnsafeSubscriber
instance is created in a different thread than the one that invokes onSubscribe()
(true for an asynchronous Publisher
), according to the java memory model, this statement inside onSubscribe()
:
if (duplicateOnSubscribe) {
is guaranteed to compute to false
if and only if the instance is published safely between these threads. None of the rules in the specifications establish a happens-before relationship between Publisher#subscribe()
and Subscriber#onSubscribe()
. So, the usage above can be categorized as unsafe. In a more convoluted form, the assignment:
private boolean duplicateOnSubscribe = false;
can be interleaved with
duplicateOnSubscribe = true;
such that duplicateOnSubscribe
is set to false
later.
Has this been considered before or am I missing something?
Activity
viktorklang commentedon Apr 3, 2020
@NiteshKant Isn't this addressed by https://github.com/reactive-streams/reactive-streams-jvm#1.3 ?
NiteshKant commentedon Apr 4, 2020
I do not think so. Take the below code as an example:
UnsafePublisher
follows rule 1.3 i.e. it makes sure that allSubscriber
methods are invoked serially. There is a happens-before relationship betweenonSubscribe()
->onNext()
due to rule 2.11 which says receive ofonSubscribe()
happens before processing ofonSubscribe()
.However, due to the unsafe publishing of
Subscriber
instance there is no happens-before betweensubscribe()
andonSubscribe()
which meansduplicateOnSubscribe
in the original code still is racy.viktorklang commentedon Apr 4, 2020
@NiteshKant Wouldn't https://github.com/reactive-streams/reactive-streams-jvm#1.9 prevent the onSubscribe to be executed on some other thread, as it would not be executed under
subscribe
?Scottmitch commentedon Apr 4, 2020
https://github.com/reactive-streams/reactive-streams-jvm#1.9 is focused on the ordering of methods invoked on the
Subscriber
(e.g.onSubscribe
must be first). The specification uses the term serial to require a "happens-before" relationship, and it isn't clear that 1.9 provides this (or if some other combination of rules is meant to imply this relationship). What I would expect is something like:<1.12, or existing rule(s)?>:
A
Publisher
MUST establish a serial relationship betweensubscribe
and its first interaction with theSubscriber
(e.g. theonSubscribe
method per 1.9).rational:
This rule is intended to clarify that any local non-final state initialized for use in the
Subscriber
before thePublisher.subscribe
call will be visible before thePublisher
interacts with aSubscriber
.NiteshKant commentedon Apr 4, 2020
I don’t think 1.9 disallows
onSubscribe()
to be called from a different thread as long as the order of method calls to theSubscriber
is as suggested by the spec. In my example I am just demonstrating how an unsafe publication ofSubscriber
can still lead to following the spec but unexpected behavior in theSubscriber
.The intent here is to see whether this aspect should be covered by the spec.
rkuhn commentedon Apr 4, 2020
So far the creation of protocol entities as well as their internal structure and communication is not mentioned in the spec. It is reasonable to assume that a Subscriber does not need to be thread-safe given the provisions in §1.9, the same as objects delivered to
onNext
are reasonably expected to be normal (unsynchronized) POJOs. In this light I’d find it reckless to use unsafe publication in the implementation of a Publisher, and if I’m not mistaken this would be outside the JMM in any case. So for me the proposed new rule can already be derived from the existing ones.@NiteshKant are you aware of any implementations that would be affected by such a new rule?
Side note: §1.9 is indeed ambiguous on whether
onSubscribe
must happen beforesubscribe
returns — it is worded as though it should (though not crystal clear), but the intent clarification detracts from this provision.NiteshKant commentedon Apr 4, 2020
@rkuhn I would agree that it will be reckless to use unsafe publication of a
subscriber
instance inside aPublisher
implementation but isn't one of the goals of a specification to ensure that implementors are not reckless? 🙂The objects delivered to
onNext
doesn't have to worry about safe-publication (if processes asynchronously from withinonNext
) as Rule 2.11 prohibits unsafe publication.I do not know of any implementation that would be negatively affected but I can point you to this change in ServiceTalk that will be positively impacted with regards to clarity in memory visibility.
Actually I thought it is intentional for the spec to allow for delayed
onSubscribe()
signal aftersubscribe()
returns. It certainly is useful to decouple sources that perform blocking work insidesubscribe()
from the caller ofsubscribe()
.viktorklang commentedon Apr 4, 2020
@rkuhn @NiteshKant You could also argue that since the spec doesn't mandate that creation of any of Publisher, Subscriber, Subscription, and Processor is safely published, the Publisher cannot assume that it can unsafely publish a Subscriber.
NiteshKant commentedon Apr 4, 2020
Yes agreed! The question here is whether we should make this explicit in the spec that a
Publisher
should safely publish theSubscriber
such that implementations ofSubscriber
s (like the ServiceTalk one I referenced above) can assume memory visibility guarantees.viktorklang commentedon Apr 4, 2020
@NiteshKant I guess we could clarify the intent of 1.9?
NiteshKant commentedon Apr 4, 2020
Yes that sounds like something that will be beneficial. Should I take a stab at the clarification?
viktorklang commentedon Apr 4, 2020
@NiteshKant Yes, please do! :)
Publisher#flatMapSingle make subscription non-volatile
21 remaining items