1.x: fix observeOn resource handling, add delayError capability #3544
Conversation
|
I like the change, but can't we ship this behavior as default? Yes, it's a breaking change, but in a good way. I mean, I'm just not sure that a lot of users will even know about this operator and even less will understand the difference and situations when they'll need to use one instead of another. |
| @@ -145,106 +150,126 @@ public void onCompleted() { | |||
| @Override | |||
| public void onError(final Throwable e) { | |||
| if (isUnsubscribed() || finished) { | |||
| RxJavaPlugins.getInstance().getErrorHandler().handleError(e); | |||
There was a problem hiding this comment.
Looks like a candidate for a separate PR, isn't it?
There was a problem hiding this comment.
More PRs to the same class ends up being merged much later and usually in the wrong order.
|
@akarnokd are you concerned by the performance impacts of this PR? |
|
No concerns. |
| ts.assertNoErrors(); | ||
| ts.assertNotCompleted(); | ||
|
|
||
| ts.requestMore(3); // requesting 2 doesn't switch to the error() source for some reason in concat. |
There was a problem hiding this comment.
Any idea why requesting 2 didn't work?
There was a problem hiding this comment.
The drain loop quits because it runs out of requests but at the same time, the concatenated data source is also empty. I can add a if (checkTerminated(finished, q.isEmpty(), localChild, q)) return check again, but this adds overhead for those sources that request again anyway.
There was a problem hiding this comment.
Ok I see, thanks for the detailed explanation.
|
👍 |
|
Since this adds to the API surface, can I have another +1? (Also feel free to merge this.) |
|
Closing and reposting again with proper rebase. |
This PR fixes the "messing around" reported in #3002 and adds an overload to
observeOnthat allows delaying errors without the need for wrapping (see #3542 and maybe there are other reports).In addition, this PR adds a proper override of the
isEmptymethod to simply compare the two indexes for emptiness directly instead ofsize() == 0to avoid looping, multi-reading and casting.Benchmark comparison (i7 4790, Windows 7 x64, Java 8u66):
Note that the benchmark is generally quite noisy, yielding hectic results (i.e., firing up a thread with newThread may take quite some random microseconds). For example,
observeOnImmediateshouldn't be affected by any of the changes yet the run-to-run variance is +/- 10%. I'm fine with the results of the benchmark.