Skip to content
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

Explain the goal of each rule in the spec #177

Closed
viktorklang opened this issue Dec 13, 2014 · 19 comments
Closed

Explain the goal of each rule in the spec #177

viktorklang opened this issue Dec 13, 2014 · 19 comments
Assignees
Milestone

Comments

@viktorklang
Copy link
Contributor

I think it could provide a lot of value to readers of the spec if every rule in it was annotated with what the rule is there to achieve/prevent.

Thoughts?

@drewhk
Copy link
Contributor

drewhk commented Dec 15, 2014

I think this is a very good idea.

@ktoso
Copy link
Contributor

ktoso commented Dec 15, 2014

Possibly relates to #145 implementor notes, could be put in there?

@viktorklang
Copy link
Contributor Author

@ktoso it definitely relates to implementor notes, but I'd like to keep it adjacent to the actual rules themselves, to make sure that it's kept up to date.

@davidmoten
Copy link
Contributor

@viktorklang Happy to contribute in my small way (spelling and whitespace a speciality). So the location of these explanations is in the notes [1],[2], etc at the bottom of each section Publisher, Subscriber, Subscription. Is that so?

@viktorklang
Copy link
Contributor Author

@davidmoten I imagined a new column (to the right) with the explanation of the rule, keeping the bits together, so to say.

@davidmoten
Copy link
Contributor

@viktorklang Just read #283 and was reminded of this little task.

Something that's been on my table for a while is to create, for each rule, a section of rationale for the rule, to assist implementors in understanding the reason behind the rules, for deeper understanding.

I knocked up a gist demoing options using markdown.

https://gist.github.com/davidmoten/03ca6332e416e050a4a6

Given the formatting limitiations of markdown I'd suggest adding explanations as an extra row rather than in a column.

@ktoso
Copy link
Contributor

ktoso commented Jul 8, 2015

The 3rd option in your gist looks pretty good actually (notes under rule)!

@viktorklang
Copy link
Contributor Author

Thanks @davidmoten!
I, too, find the 3rd option the most viable. My only fear is that it may make it harder to visually scan the rules, but we'll solve that problem if it rears its ugly head, for sure.

@anthonyvdotbe
Copy link
Contributor

This would be immensely useful. I got here through Java 9's corresponding Flow API, but don't have any experience with implementations such as Reactor, RxJava or Akka. So for people like me, this would be a great addition.

What's the preferred way to suggest explanations to be added? I have a few more questions concerning the spec/README. Can I add them as a comment here, or is it preferred I file new issues for these (maybe with a specific label, since they're really just questions to get a better understanding of the spec)?

@akarnokd
Copy link
Contributor

akarnokd commented Apr 8, 2016

Ask away!

@anthonyvdotbe
Copy link
Contributor

Thanks for the invitation @akarnokd. Here goes:

2.6 A Subscriber MUST call Subscription.cancel() if it is no longer valid to the Publisher without the Publisher having signaled onError or onComplete.

What does it mean for a Subscriber to be "valid to the Publisher"?

2.11 A Subscriber MUST make sure that all calls on its onXXX methods happen-before [1] the processing of the respective signals. I.e. the Subscriber must take care of properly publishing the signal to its processing logic.

I'm not sure I understand this. From the mentions in the example AsyncSubscriber, I'd guess it means that a Subscriber MAY asynchronously process its signals, but MUST process them one at a time, with a happens-before relationship between the "end of processing signal N" and the "start of processing signal N+1". Is this correct?

2.13 Calling onSubscribe, onNext, onError or onComplete MUST return normally [...]

Is it allowed for an onXxx method to throw an Error (e.g. OutOfMemoryError)? The comment of @smaldini in #292 suggests to me it is, but I was unable to find this mention of "non-fatal" in the spec. Moreover, the examples always catch Throwable, suggesting that it is indeed not allowed. Also, there is a note saying

The term "return normally" means "only throws exceptions that are explicitly allowed by the rule".

but it's unclear to me if "exceptions" refers to java.lang.Exception or java.lang.Throwable.

2.13 [...] and the caller MUST raise this error condition in a fashion that is adequate for the runtime environment.

When is something "adequate for the runtime environment"? I assume it's always adequate to simply not handle such exceptions at all, right?

3.1 Subscription.request and Subscription.cancel MUST only be called inside of its Subscriber context. A Subscription represents the unique relationship between a Subscriber and a Publisher [see 2.12].

What does "inside of its Subscriber context" mean here? For example, say I have a Subscriber which passes its Subscription to an instance of class X, which does nothing other than invoking the Subscription's cancel() method after a certain time. Would this be illegal, i.e. is X considered to be outside of the Subscription's Subscriber context?

In the section "Asynchronous vs Synchronous Processing", the first sentence says:

The Reactive Streams API prescribes that all processing of elements (onNext) or termination signals (onError, onComplete) MUST NOT block the Publisher.

However, there's no occurrence in the specification of the term "block" and in the rules about responsivity (2.3, 3.4, 3.5), only 3.5 says Subscription.cancel MUST respect the responsivity of its caller, while the others use RECOMMENDED/SHOULD.

The section "Asynchronous vs Synchronous Processing" also mentions:

[...] within the bounds of a non-blocking, asynchronous, push-based stream.

Why does it say push-based here, and not pull-based? Since the Subscriber explicitly requests items, I'd say this is a pull-based mechanism, no? What am I missing here?

@akarnokd
Copy link
Contributor

akarnokd commented Apr 8, 2016

§2.6: That sentence doesn't make any sense to me.

§2.11: Not sure, it probably means the need for barriers before going async with the supplied parameter to onXXX. In practice, we had to make sure there was no reordering in the sequence over an async boundary.

§2.13: indeed, the spec is vague on this one. In practice, we had to make exceptions to some Exceptions and propagate them even if catched as a Throwable. A great candidate to fix in the next version of the spec.

§2.13: That's an escape hatch in the spec. The example prints the stacktrace to the console in case there is an exception that can't be delivered (including fatal ones). In practice, we have something like a catch-all sink for undeliverable exceptions where a global handler can get them anyway. Dropping them is an option but one could miss subtle errors.

§3.1: Not sure what it meant. In practice, exposing a Subscription outside its target Subscriber can lead to race condition with the Subscriber's own logic. First, the Subscription itself may arrive late and thus you can't cancel it or request values from it upfront. Second, issuing extra request could overflow the Subscriber's logic or cancelling the upstream may leave the Subscriber hang and leaking resources.

"not block": sounds a bit harsh. It is supposed to indicate that one should avoid sleeping, waiting on an object or condition, because those can be written in a notification-like (i.e., push) fashion.

"push-based": the fact that the Subscriber request values doesn't mean that those values are immediately available. In a pull-based API, like Iterable, when you call next(), it can't return saying "sorry, no value yet but may come later"; it has to return a valid value or throw NoSuchElementException but still has to wait for the next value to become available in some way (i.e., waiting on a blocking queue). This comes from the extended Observable-Iterable duality. In Iterable terms, you'd have an available() method somewhere that tells how many times you could call onNext() without blocking (see InputStream).

@anthonyvdotbe
Copy link
Contributor

Thanks @akarnokd!

§2.11: Not sure, it probably means the need for barriers before going async with the supplied parameter to onXXX. In practice, we had to make sure there was no reordering in the sequence over an async boundary.

I'm afraid I don't understand this explanation. What does it mean to "go async" with the parameter?
And about the reordering: does that mean it would be illegal for AsyncSubscriber to use a PriorityBlockingQueue instead of a ConcurrentLinkedQueue? Where an onSubscribe signal would have highest priority, an onComplete/onError signal lowest priority, and an onNext signal something in between, based on the parameter? In other words: onNext items are processed based on priority, and not in the order as delivered by the Publisher?

@viktorklang
Copy link
Contributor Author

@anthonyvdotbe

What's the preferred way to suggest explanations to be added?

How about creating corresponding Issues per Rule paired with questions, then we can refine the explanation in the Issue and create PRs from them?

@viktorklang
Copy link
Contributor Author

@anthonyvdotbe

§2.6:

If the "downstream" completes or errors before the "upstream". Imagine for instance a Processor which only wants the first element from its "upstream". Or a Subscriber which receives a "forbidden" value. etc.

The intent of this rule is to make sure that Publishers will be able to do resource cleanup when they are no longer needed.

§2.11:

This Rule intends to ensure that safe publication of Subscriber signals are the responsibility of the Subscriber.

§2.13:

The intent of the rule is to make sure nobody uses Exception throwing as signalling of anything but fatal errors, where fatal is to be considered failure conditions who are not recoverable and require program termination.

As for exceptions and errors, please see this first sentence of the java.lang.Error javadoc:

"An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch."

As for "raise this error condition in a fashion that is adequate for the runtime environment." means that instead of rethrowing the exception it should use whatever mechanisms provided to it, this may include things like logging.

§3.1:

See §2.7 for more background. Linking to §2.7 from §3.1 seems like a good idea.

As for:

The Reactive Streams API prescribes that all processing of elements (onNext) or termination signals (onError, onComplete) MUST NOT block the Publisher.

"block" may not have been the best word, and it should most definitely be explained, but the intent is that a Subscriber should not impede the progress of the Publisher from an execution PoV. I.e. the Subscriber should not starve the Publisher from CPU cycles.

As for:

Why does it say push-based here, and not pull-based? Since the Subscriber explicitly requests items, I'd say this is a pull-based mechanism, no? What am I missing here?

Good catch, I think this should say "dynamic push-pull stream"

@viktorklang
Copy link
Contributor Author

@anthonyvdotbe

I'm afraid I don't understand this explanation. What does it mean to "go async" with the parameter?

Notice how the methods on subscriber is referred to as "signals". This means that invoking the onX methods (signalling something) is divorced from the processing of said signal. Hence that signal can either be processed synchronously, or asynchronously.

@anthonyvdotbe
Copy link
Contributor

@viktorklang

How about creating corresponding Issues per Rule paired with questions, then we can refine the explanation in the Issue and create PRs from them?

I've just created issue #321 for §2.6 I will create analog issues for other things that came up in the above comments. If you'd like me to change the title/labels or something, just let me know.

@viktorklang
Copy link
Contributor Author

@anthonyvdotbe That's perfect, thank you!

@viktorklang
Copy link
Contributor Author

Apologies for the delay here, fixed in #339

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

6 participants