reactor / reactor-core

Non-Blocking Reactive Foundation for the JVM
http://projectreactor.io
Apache License 2.0
4.98k stars 1.21k forks source link

Passing null to Subscriber#onNext, is this not a TCK violation? #3157

Closed be-hase closed 2 years ago

be-hase commented 2 years ago

(Sorry, let me ignore the issue template as it may not be a bug.)

In my project, I wrap(deletegate) CoreSubscriber using ReactorHooks for tracing and MDC propagation. It has a similar implementation to spring sleuth. https://github.com/spring-cloud/spring-cloud-sleuth/blob/c795675cf93c666c3c13f9bbb9eadbde1f1aa775/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/reactor/ReactorSleuth.java#L666-L669

However, my code execute kotlin's non-null assertion.

// e.g.
override fun onNext(t: Any) { // kotlin's non-null assertion
    delegate.onNext(t)
}

The other day, when I upgraded the reactor version, a problem started to occur. Because with this change null is now passed to onNext. https://github.com/reactor/reactor-core/commit/bee3d07a3f7ddc6faf7563285bbce9759ff4f841#diff-8869bae039aadb2cabfd1edc66079b5aee45606de2374390f19d2cd7356de25cR278

I understand that reactor optimizes performance with Fuseable, but I am uninformed about the details. On the other hand, according to TCK in reactive streams, it seems to expect a NullPointerException when passing null to onNext. https://github.com/reactive-streams/reactive-streams-jvm/blob/944163a4b2477a2bebaaada86b0ba910b6302f2f/tck/src/main/java/org/reactivestreams/tck/SubscriberWhiteboxVerification.java#L390 https://github.com/reactive-streams/reactive-streams-jvm#2.13:~:text=parameter%20is%20null%20in%20which%20case%20it-,MUST%20throw%20a%20java.lang.NullPointerException%20to%20the%20caller,-%2C%20for%20all%20other%20situations%20the%20only%20legal

Wouldn't this be a violation of TCK? (Allowing null in my code solves the problem, but I am wondering and would like to know.)

OlegDokuka commented 2 years ago

@be-hase the problem is with your CoreSubscriber implementation. Once you implement our internal interface, you MUST follow all the internal world rules. Within the project reactor, we have a notion of fusion (internal operators optimization). Once ASYNC fusion mode is obtained, values it selves stop being propagated, but rather a special notification (null) caries information through onNext chain about the fact that some values are queued and waiting for draining.

That all says the null value is expected when you start pretending to be internal thing and the best fix would be allowing null values.

be-hase commented 2 years ago

Thank you! Very helpful, I now have a better understanding of reactor.

Once you implement our internal interface, you MUST follow all the internal world rules.

What you say is very true. I will fix my implementation.

As a result, sorry for asking the question in the Github issue. Thanks.