2.x: Improve JavaDoc of XObserver types. #5841

akarnokd · 2018-02-05T12:34:26Z

This PR improves the JavaDoc of the 4 RxJava main consumer types:

Observer
SingleObserver
MaybeObserver
CompletableObserver

codecov · 2018-02-05T12:50:19Z

Codecov Report

Merging #5841 into 2.x will increase coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                2.x    #5841      +/-   ##
============================================
+ Coverage     96.38%   96.41%   +0.02%     
- Complexity     5812     5819       +7     
============================================
  Files           634      634              
  Lines         41760    41760              
  Branches       5796     5796              
============================================
+ Hits          40251    40263      +12     
+ Misses          591      579      -12     
  Partials        918      918

Impacted Files	Coverage Δ	Complexity Δ
.../io/reactivex/internal/functions/ObjectHelper.java	`94.73% <0%> (-5.27%)`	`20% <0%> (-1%)`
...erators/completable/CompletableConcatIterable.java	`95.91% <0%> (-4.09%)`	`2% <0%> (ø)`
...rnal/operators/flowable/FlowableFlatMapSingle.java	`92.93% <0%> (-3.81%)`	`2% <0%> (ø)`
.../internal/operators/flowable/FlowableInterval.java	`94.44% <0%> (-2.78%)`	`3% <0%> (ø)`
...tivex/internal/schedulers/TrampolineScheduler.java	`92.3% <0%> (-2.57%)`	`6% <0%> (ø)`
...l/operators/observable/ObservableFlatMapMaybe.java	`87.58% <0%> (-1.97%)`	`2% <0%> (ø)`
.../operators/observable/ObservableFlatMapSingle.java	`88.8% <0%> (-1.5%)`	`2% <0%> (ø)`
...ivex/internal/operators/maybe/MaybeMergeArray.java	`96.62% <0%> (-1.13%)`	`6% <0%> (ø)`
...nternal/operators/observable/ObservableCreate.java	`98.29% <0%> (-0.86%)`	`2% <0%> (ø)`
.../operators/maybe/MaybeFlatMapIterableFlowable.java	`97.54% <0%> (-0.82%)`	`2% <0%> (ø)`
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 363f038...41f74b9. Read the comment docs.

artem-zinnatullin

Great rewrite! But comment about errors from onX methods

artem-zinnatullin · 2018-02-06T05:45:04Z

src/main/java/io/reactivex/Observer.java

+ * Calling {@link #onSubscribe(Disposable)}, {@link #onNext(Object)} or {@link #onError(Throwable)} with a
+ * {@code null} argument is forbidden.
+ * <p>
+ * The implementations of the {@code onXXX} methods should avoid throwing runtime exceptions other than the following cases:


Another valid case is fatal business logic violation. Otherwise people might think that swallowing errors is preferred because doc said so

What do you mean by "fatal business logic violation"?

When for some reason Observer finds itself in inconsistent state during onXXX() execution.

In this case exception thrown from onXXX() will reach RxJavaPlugins as OnErrorNotImplementedException and in case of Android will crash the application, on JVM user can hook into that and specify required behavior.

This is fail fast design principle, when instead of swallowing error states and writing defensive code you fail as soon as possible preventing inconsistent states from going deeper into your system.

I'm just afraid that this doc might discourage people from that design principle

In this case exception thrown from onXXX() will reach RxJavaPlugins as OnErrorNotImplementedException and in case of Android will crash the application, on JVM user can hook into that and specify required behavior.

That is the property of the lambda-subscribe methods and not when using a plain Observer.

The sentence is the derivative of the Reactive Streams rule 2.13 and is intended to discourage throwing from these methods as they may not end up where users expect it. Remember those issues where the sync code propagated the exception on the main thread but as soon as asynchrony was involved, no errors were suddenly thrown by the unit test. So don't throw from Observer methods and you'll be fine.

That is the property of the lambda-subscribe methods and not when using a plain Observer.

Ah right, but exception is still delivered to RxJavaPlugins' error handler.

The sentence is the derivative of the Reactive Streams rule 2.13 and is intended to discourage throwing from these methods as they may not end up where users expect it. Remember those issues where the sync code propagated the exception on the main thread but as soon as asynchrony was involved, no errors were suddenly thrown by the unit test.

Yep, RS 2.13, yep RxJava 2.x broke unit testing. However I still think throwing exception from onXXX is okay if system finds itself in inconsistent state.

RS 2.13 says:

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.

Specifically:

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.

Which actually sort of allows throwing exceptions from onXXX() since RxJava indeed provides a way to raise this error condition in a fashion that is adequate for the runtime environment.

What I want to say is that there are thousands of projects where exceptions happen in onXXX() methods, crash apps and then appear as fatal errors in bug tracking systems.

Correct solution will be to fix such errors by fixing the code that produces them in the first place rather than somehow not throwing them.

It's also unclear how one should avoid throwing exceptions from onXXX() if Java has unchecked exceptions and some JVM languages like Kotlin treat every exception as unchecked. I hope you don't expect people to wrap code in every onXXX() callback into a try/catch :)

There is no guarantee where such exceptions will end up:

if the flow is synchronous, the subscribe will end up throwing

if there is asynchrony, depending on the scheduler, the error may end up in

RxJavaPlugins.onError,

The current thread's uncaught exception handler directly, or

get swallowed in a custom scheduler implementation

From the Observable's perspective, an Observer is the end consumer thus it is the Observer's responsibility to "crash further down". Yes, this means unreliable code should be wrapped into try-catch, specifically in onError or onComplete. If there is a crash in onNext, as the rule says, dispose the Disposable and signal the error by some other means, for example, by calling this.onError().

Added this type of clarification to the JavaDoc.

vanniktech

The documentation is getting slicker and slicker.

vanniktech · 2018-02-06T10:05:49Z

src/main/java/io/reactivex/CompletableObserver.java

+ * <p>
+ * When a {@code CompletableObserver} is subscribed to a {@link CompletableSource} through the {@link CompletableSource#subscribe(CompletableObserver)} method,
+ * the {@code CompletableSource} calls {@link #onSubscribe(Disposable)}  with a {@link Disposable} that allows
+ * cancelling the sequence at any time. A well-behaved


should we use cancelling here or disposing since the method is called dispose?

vanniktech · 2018-02-06T10:06:21Z

src/main/java/io/reactivex/CompletableObserver.java

+ * When a {@code CompletableObserver} is subscribed to a {@link CompletableSource} through the {@link CompletableSource#subscribe(CompletableObserver)} method,
+ * the {@code CompletableSource} calls {@link #onSubscribe(Disposable)}  with a {@link Disposable} that allows
+ * cancelling the sequence at any time. A well-behaved
+ * {@code CompletableSource} will call a {@code CompletableObserver}'s {@link #onError(Throwable)}


I'd use may call a since you can also just call onSubscribe and then do nothing.

Technically yes, but we don't want to encourage that for end users because they'll just hang.

Alright works for me.

vanniktech · 2018-02-06T10:07:01Z

src/main/java/io/reactivex/CompletableObserver.java

+ * happens, it is the duty of the {@code CompletableObserver} implementation to be ready to receive multiple calls to
+ * its methods and ensure proper concurrent behavior of its business logic.
+ * <p>
+ * Calling {@link #onSubscribe(Disposable)} or{@link #onError(Throwable)} with a


I think there's a space missing between or and {

artem-zinnatullin

I don't really agree with wording about try/catch, but also don't want to block this PR because it's 99% awesome

2.x: Improve JavaDoc of XObserver types.

1324f26

akarnokd added Documentation 2.x labels Feb 5, 2018

akarnokd added this to the 2.2 milestone Feb 5, 2018

artem-zinnatullin reviewed Feb 6, 2018

View reviewed changes

vanniktech approved these changes Feb 6, 2018

View reviewed changes

Use "disposing", add missing space

a11f228

vanniktech approved these changes Feb 7, 2018

View reviewed changes

Add more explanation about throwing exceptions.

41f74b9

artem-zinnatullin approved these changes Feb 8, 2018

View reviewed changes

akarnokd merged commit f7a2870 into ReactiveX:2.x Feb 8, 2018

akarnokd deleted the JavadocClarifications0205 branch February 8, 2018 22:02

akarnokd mentioned this pull request Feb 19, 2018

2.1.10 release preparations #5859

Closed

jared-bot mentioned this pull request Feb 25, 2018

Dependency update: RxJava xmartlabs/bigbang-template#62

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2.x: Improve JavaDoc of XObserver types. #5841

2.x: Improve JavaDoc of XObserver types. #5841

akarnokd commented Feb 5, 2018

codecov bot commented Feb 5, 2018 •

edited

Loading

artem-zinnatullin left a comment

artem-zinnatullin Feb 6, 2018

akarnokd Feb 6, 2018

artem-zinnatullin Feb 7, 2018

akarnokd Feb 7, 2018

artem-zinnatullin Feb 7, 2018

akarnokd Feb 7, 2018

akarnokd Feb 7, 2018

vanniktech left a comment

vanniktech Feb 6, 2018

vanniktech Feb 6, 2018

akarnokd Feb 6, 2018

vanniktech Feb 6, 2018

vanniktech Feb 6, 2018

artem-zinnatullin left a comment

2.x: Improve JavaDoc of XObserver types. #5841

2.x: Improve JavaDoc of XObserver types. #5841

Conversation

akarnokd commented Feb 5, 2018

codecov bot commented Feb 5, 2018 • edited Loading

Codecov Report

artem-zinnatullin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanniktech left a 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

artem-zinnatullin left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 5, 2018 •

edited

Loading