Clarify the definition of the term "return normally"

[anthonyvdotbe]

The note says:

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

but "exceptions" is too vague here. In my opinion, this must be reworded to use specific class names. I've asked about this in the context of §2.13 and based on the replies I propose the following:

The term "return normally" means "only throws instances of java.lang.RuntimeException that are explicitly allowed by the rule, or instances of java.lang.Error".

[viktorklang]

@anthonyvdotbe Your proposal would possibly be backwards incompatible.

A backwards compatible alternative would be something along the lines of:

The term "return normally" means "only throws instances of java.lang.Throwable that are either explicitly allowed by the rule or signal that a fatal error has occurred.".

Thoughts @reactive-streams/contributors ?

[anthonyvdotbe]

The term "return normally" means "only throws instances of java.lang.Throwable that are either explicitly allowed by the rule or signal that a fatal error has occurred.".

If I'd come across this definition, my train of thoughts would be something like:

• fatal error == java.lang.Error, obvious
• but then why didn't the spec authors use java.lang.Error to avoid all ambiguity?
• maybe "fatal error" is meant to be interpreted as "unchecked exception" here?
• but then why didn't they use that instead, since the JLS unambiguously defines that term?

And I'd still be puzzled about whether or not RuntimeExceptions that are not explicitly allowed, are considered "fatal errors" (or may be considered as such under certain conditions). Or even if all instances of java.lang.Error are considered "fatal", e.g. what about java.lang.NoClassDefFoundError? (Also there's the contrived case of using Class::newInstance, which might even cause checked exceptions to be thrown as well.)

So personally I would prefer an unambiguous definition, even if it might introduce an incompatibility. At least you'd know for sure that the implementation is correct/incorrect, whereas now an implementation may be both correct and incorrect at the same time, depending on the definition of "fatal error". Just my 2 cents.

[viktorklang]

How about: "Fatal error" means that a failure so severe has happened that the only sensible course of action is to crash the virtual machine process.

[anthonyvdotbe]

I'd really like the specification to define "returns normally" in terms of specific class names. Given your definition of "fatal error", I also fail to see why:

[...] or are instances of java.lang.Error

would possibly be backward incompatible, while:

[...] or signal that a fatal error has occurred

would not?

[viktorklang]

@anthonyvdotbe Why class names? (Not all Errors are fatal, and not all fatal failures are java.lang.Errors.)

[anthonyvdotbe]

@viktorklang because I'm wondering: who decides what a fatal error is and is not? In other words: if I write a Subscriber implementation, how can I be sure I'm implementing the spec correctly, and rule 2.13 will never be violated? Or is this actually a non-question and do I don't have to care about which unchecked exceptions my implementation may throw, since the spec clarifies what the caller must do if the rule is violated? Or should I have a try { ... } catch(Throwable t) { ... } in all my methods, and simply define fatal error as "any error that occurs while trying to handle the Throwable in the catch block"? Also, in my experience it's very atypical to catch unchecked exceptions or Throwable (unless maybe to add suppressed exceptions, like try-with-resources does). On the other hand, examples I've seen thus far all catch Throwable in their Subscriber implementations. So if this is indeed required for writing a compliant Subscriber, I believe the spec should clearly indicate this, because casual readers will easily miss something "unintuitive" like this.

[viktorklang]

@anthonyvdotbe There is no way to guarantee that rules will never be violated. :)
Since the main interaction is the Publisher calling signals on the Subscriber, if those fail, there's really not much the publisher can do. By saying that if there is a throwable thrown by the Subscriber, the Subscriber has violated the spec, so we need to log that somehow so it is not just a silent bug.
So:

Calling onSubscribe, onNext, onError or onComplete MUST return normally except when any provided parameter is null in which case it MUST throw a java.lang.NullPointerException to the caller, for all other situations the only legal way for a Subscriber to signal failure is by cancelling its Subscription. In the case that this rule is violated, any associated Subscription to the Subscriber MUST be considered as cancelled, and the caller MUST raise this error condition in a fashion that is adequate for the runtime environment.

As for fatalities, it is about Throwables where the catcher deems them as being indicative of irrecoverable system failures, i.e. warranting a System.exit(nonzero).

[anthonyvdotbe]

Ok, I think I get it. Actually, I'd just change the note to the following then:

The term "return normally" means "only throws instances of java.lang.Throwable that are explicitly allowed by the rule".

and add a note to §2.13 saying something like:

an adequate fashion may be, for example, logging the exception. Or, for Throwables that are deemed indicative of irrecoverable system failures, halting/crashing the VM.

@viktorklang what do you think?

[viktorklang]

@anthonyvdotbe Doesn't have to ber something like:

The term "return normally" means "MOST ONLY return a value or throws instances of java.lang.Throwable that are explicitly allowed by the rule".

?

I like your footnote.

[anthonyvdotbe]

@viktorklang I had simply replaced exceptions with instances of java.lang.Throwable in the actual note. But yes, upon rereading it, I'd say it's not an improvement over the current wording.
I think the use of return a value in your proposal may cause some confusion, since all methods are defined to return void.

Bottom line: I believe this issue can be closed without change, and the addition of the note to §2.13 can be taken care of in #328

[viktorklang]

@anthonyvdotbe Deal :)