Modifies rules 1.09 and 2.13 to mandate `java.lang.NullPointerException`... #209

viktorklang · 2015-02-04T13:54:01Z

... be thrown.

Updates the TCK, Spec and example implementations.

@reactive-streams/contributors As discussed in #204, here is my proposed spec, tck, example amendment for null handling. Please comment and vote before the 9th of February 2015 as we should attempt to ship 1.0.0.RC2 STAT.

viktorklang · 2015-02-04T14:09:36Z

README.md

@@ -65,6 +65,7 @@ onError | (onSubscribe onNext* (onError | onComplete)?)
 - The specifications below use binding words in capital letters from https://www.ietf.org/rfc/rfc2119.txt
 - The terms `emit`, `signal` or `send` are interchangeable. The specifications below will use `signal`.
 - The terms `synchronously` or `synchronous` refer to executing in the calling `Thread`.
+- The term "return normally" means "only throws exceptions that are explicitly allowed by the rule".


Does this need to be even clearer as in:

"is only allowed to throw exceptions that are explicitly allowed by the rule"

benjchristensen · 2015-02-04T17:42:50Z

README.md

@@ -119,7 +120,7 @@ public interface Subscriber<T> {
 | <a name="2.10">10</a>     | A `Subscriber` MUST be prepared to receive an `onError` signal with or without a preceding `Subscription.request(long n)` call. |
 | <a name="2.11">11</a>     | A `Subscriber` MUST make sure that all calls on its `onXXX` methods happen-before [[1]](#footnote-2-1) the processing of the respective signals. I.e. the Subscriber must take care of properly publishing the signal to its processing logic. |
 | <a name="2.12">12</a>     | `Subscriber.onSubscribe` MUST be called at most once for a given `Subscriber` (based on object equality). |
-| <a name="2.13">13</a>     | Calling `onSubscribe`, `onNext`, `onError` or `onComplete` MUST return normally. 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. |
+| <a name="2.13">13</a>     | Calling `onSubscribe`, `onNext`, `onError` or `onComplete` MUST return normally except in the case where any provided parameter is `null` in which case it MUST throw a `java.lang.NullPointerException` to the caller, this aside 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. |


I don't understand the grammar of "this aside". I read this as meaning "this being the only legal way ...".

@benjchristensen Thanks for sanity-checking my English! You are indeed right.

What do you think about:

". Besides this, the only legal way for a Subscriber to signal failure is by cancelling its Subscription."?

@benjchristensen Or perhaps even clearer:

" to the caller, for all other situtations the only legal way for a Subscriber to signal failure is by cancelling its Subscription."

@reactive-streams/contributors Any other suggestions here? :)

I'm okay with -> " to the caller, for all other situtations the only legal way for a Subscriber to signal failure is by cancelling its Subscription."

Excellent, thanks for pointing this out!

benjchristensen · 2015-02-04T17:43:29Z

I'm 👍 on the changes other than the wording comment I added.

viktorklang · 2015-02-05T13:35:05Z

Thanks @benjchristensen!

ktoso · 2015-02-05T16:47:26Z

tck/src/main/java/org/reactivestreams/tck/SubscriberWhiteboxVerification.java

+        env.verifyNoAsyncErrors();
+      }
+    });
+  }


simplicity wins over duplication here I guess (whitebox + blackbox), is fine as-is I'd say.

ktoso · 2015-02-05T19:15:43Z

Looks good 👍

Though it's missing the "anti tck-regression tests" we've been keeping recently, whipped them up for you here @viktorklang: ktoso@dade909#diff-f058033d69037e8082bda05ee49d781aR144 Feel free to cherry pick or include at will. (splits test in 3 parts, so it's actually testable for these separate messages).

viktorklang · 2015-02-05T22:07:01Z

@ktoso Thanks Konrad, I amended my commit with your update!

ktoso · 2015-02-05T22:36:28Z

You're welcome :) LGTM

…on` be thrown. Updates the TCK, Spec and example implementations.

viktorklang · 2015-02-06T20:07:25Z

README.md

@@ -86,7 +87,7 @@ public interface Publisher<T> {
 | <a name="1.6">6</a>       | If a `Publisher` signals either `onError` or `onComplete` on a `Subscriber`, that `Subscriber`’s `Subscription` MUST be considered cancelled. |
 | <a name="1.7">7</a>       | Once a terminal state has been signaled (`onError`, `onComplete`) it is REQUIRED that no further signals occur. |
 | <a name="1.8">8</a>       | If a `Subscription` is cancelled its `Subscriber` MUST eventually stop being signaled. |
-| <a name="1.9">9</a>       | Calling `Publisher.subscribe` MUST return normally. The only legal way to signal failure (or reject a `Subscriber`) is via the `onError` method. |
+| <a name="1.9">9</a>       | Calling `Publisher.subscribe` MUST return normally except when the provided `Subscriber` is `null` in which case it MUST throw a `java.lang.NullPointerException` to the caller, for all other situations the only legal way to signal failure (or reject a `Subscriber`) is via the `onError` method. |


@benjchristensen Wdyt?

viktorklang · 2015-02-09T09:11:03Z

@reactive-streams/contributors Merging without objection :)

Modifies rules 1.09 and 2.13 to mandate `java.lang.NullPointerException`...

smaldini · 2015-02-09T12:10:43Z

RC2 or wait for the onSubscribe|onComplete|onError sequence resolution ?

viktorklang · 2015-02-09T12:16:57Z

@smaldini I was hoping we'd be able to solve the onSubscribe|onComplete|onError before we cut RC2.
Want to help out getting that done?
It seems like the current majority opinion is that we should aim for onSubscribe ~ onNext* ~ (onError | onComplete)

(I don't think we ought to, or need to, put a "dummy subscription" in the RS project.)

smaldini · 2015-02-09T13:33:59Z

I'm waiting for consensus and then go RC2. But yeah I'm for that too, I think @rkuhn and I shared the concern by necessity (meaning facing not nice issues when implementing a more relaxed version). And I don't think this brings much more to bypass it. It seems we already have a consensus for it anyway.

smaldini · 2015-02-09T13:34:34Z

I was about to unleash a reactor RC1 too but I can wait slightly more to be sure I'm in sync with the RS dep :)

smaldini · 2015-02-09T13:48:08Z

But yeah I'm standing for the community order to issue the release

Sent from my iPhone

On 9 Feb 2015, at 12:16 pm, Viktor Klang (√) [email protected] wrote:

@smaldini I was hoping we'd be able to solve the onSubscribe|onComplete|onError before we cut RC2.
Want to help out getting that done?
It seems like the current majority opinion is that we should aim for onSubscribe ~ onNext* ~ (onError | onComplete)

—
Reply to this email directly or view it on GitHub.

viktorklang reviewed Feb 4, 2015
View reviewed changes

viktorklang force-pushed the wip-204-spec-nulls-√ branch from e85c7b9 to 46cdc0a Compare February 4, 2015 14:12

benjchristensen reviewed Feb 4, 2015
View reviewed changes

ktoso reviewed Feb 5, 2015
View reviewed changes

viktorklang force-pushed the wip-204-spec-nulls-√ branch from 46cdc0a to 4fbf91c Compare February 5, 2015 22:04

viktorklang force-pushed the wip-204-spec-nulls-√ branch from 4fbf91c to 9ed52b9 Compare February 6, 2015 19:36

Modifies rules 1.09 and 2.13 to mandate `java.lang.NullPointerExcepti…

e2df130

…on` be thrown. Updates the TCK, Spec and example implementations.

viktorklang force-pushed the wip-204-spec-nulls-√ branch from 9ed52b9 to e2df130 Compare February 6, 2015 20:07

viktorklang reviewed Feb 6, 2015
View reviewed changes

viktorklang added this to the 1.0.0.RC2 milestone Feb 8, 2015

viktorklang added the enhancement label Feb 8, 2015

viktorklang self-assigned this Feb 8, 2015

viktorklang added a commit that referenced this pull request Feb 9, 2015

Merge pull request #209 from reactive-streams/wip-204-spec-nulls-√

9333df9

Modifies rules 1.09 and 2.13 to mandate `java.lang.NullPointerException`...

viktorklang merged commit 9333df9 into master Feb 9, 2015

viktorklang deleted the wip-204-spec-nulls-√ branch February 9, 2015 09:11

danielesegato mentioned this pull request Oct 27, 2016

2.x: Handling null values ReactiveX/RxJava#4644

Closed

moderakh mentioned this pull request Mar 1, 2021

ThroughputControl-Global Azure/azure-sdk-for-java#19183

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifies rules 1.09 and 2.13 to mandate `java.lang.NullPointerException`... #209

Modifies rules 1.09 and 2.13 to mandate `java.lang.NullPointerException`... #209

viktorklang commented Feb 4, 2015

viktorklang Feb 4, 2015

benjchristensen Feb 4, 2015

viktorklang Feb 5, 2015

viktorklang Feb 6, 2015

viktorklang Feb 6, 2015

benjchristensen Feb 6, 2015

viktorklang Feb 6, 2015

benjchristensen commented Feb 4, 2015

viktorklang commented Feb 5, 2015

ktoso Feb 5, 2015

ktoso commented Feb 5, 2015

viktorklang commented Feb 5, 2015

ktoso commented Feb 5, 2015

viktorklang Feb 6, 2015

benjchristensen Feb 6, 2015

viktorklang commented Feb 9, 2015

smaldini commented Feb 9, 2015

viktorklang commented Feb 9, 2015

smaldini commented Feb 9, 2015

smaldini commented Feb 9, 2015

smaldini commented Feb 9, 2015

Modifies rules 1.09 and 2.13 to mandate java.lang.NullPointerException... #209

Modifies rules 1.09 and 2.13 to mandate java.lang.NullPointerException... #209

Conversation

viktorklang commented Feb 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjchristensen commented Feb 4, 2015

viktorklang commented Feb 5, 2015

Choose a reason for hiding this comment

ktoso commented Feb 5, 2015

viktorklang commented Feb 5, 2015

ktoso commented Feb 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viktorklang commented Feb 9, 2015

smaldini commented Feb 9, 2015

viktorklang commented Feb 9, 2015

smaldini commented Feb 9, 2015

smaldini commented Feb 9, 2015

smaldini commented Feb 9, 2015

Modifies rules 1.09 and 2.13 to mandate `java.lang.NullPointerException`... #209

Modifies rules 1.09 and 2.13 to mandate `java.lang.NullPointerException`... #209