2.x: Coverage improvements, logical fixes and cleanups 03/08 #5905
Conversation
| t.onSubscribe(rp); | ||
|
|
||
| boolean doReplay = true; |
There was a problem hiding this comment.
The old logic could have made the Subscriber visible too early and thus it could receive an onComplete before an onSubscribe.
| return; | ||
| } | ||
| } | ||
| BackpressureHelper.addCancel(requested, n); |
There was a problem hiding this comment.
Use the helper that was not available the time this class was developed.
|
|
||
| while (j < s && r > 0) { | ||
| while (j < s && e != r) { |
There was a problem hiding this comment.
Use the now more modern emitted-requested tracking pairs.
| @@ -364,15 +370,12 @@ public void replay() { | |||
| } | |||
| } | |||
|
|
|||
| if (valuesProduced != 0) { | |||
| BackpressureHelper.producedCancel(rq, valuesProduced); | |||
There was a problem hiding this comment.
Having a separate emitted, there is no need to decrement the requested amount, saving on an atomic instruction.
| return queue.isEmpty(); | ||
| } | ||
| return !it.hasNext(); | ||
| return current == null && queue.isEmpty(); |
There was a problem hiding this comment.
An iterator is always checked for hasNext before the control is given back in poll and current is always null if the previous iterator had no further elements. Therefore, the in the original code !it.hasNext() was always false.
| @@ -1215,7 +1215,8 @@ public void subscribe(Subscriber<? super T> child) { | |||
| buf = bufferFactory.call(); | |||
| } catch (Throwable ex) { | |||
| Exceptions.throwIfFatal(ex); | |||
| throw ExceptionHelper.wrapOrThrow(ex); | |||
| EmptySubscription.error(ex, child); | |||
There was a problem hiding this comment.
This is a subscribe implementation which should not crash but signal the problem to the Subscriber.
| InnerDisposable[] ps = observers.getAndSet(TERMINATED); | ||
| if (ps != TERMINATED) { | ||
| current.compareAndSet(PublishObserver.this, null); | ||
| InnerDisposable[] ps = observers.getAndSet(TERMINATED); |
There was a problem hiding this comment.
Bias the dispose logic towards a more likely single disposal.
|
Changes explained. |
Codecov Report
@@ Coverage Diff @@
## 2.x #5905 +/- ##
============================================
+ Coverage 97.92% 98.05% +0.12%
- Complexity 5987 5995 +8
============================================
Files 655 655
Lines 43925 43920 -5
Branches 6087 6085 -2
============================================
+ Hits 43014 43065 +51
+ Misses 281 253 -28
+ Partials 630 602 -28
Continue to review full report at Codecov.
|
| for (int i = 0; i < TestHelper.RACE_DEFAULT_LOOPS; i++) { | ||
| final Flowable<Integer> cache = Flowable.range(1, 500).cache(); | ||
|
|
||
| final TestSubscriber<Integer> to1 = new TestSubscriber<Integer>(); |
There was a problem hiding this comment.
There are at least 178 such unrenamed local variables. I'll post a separate PR fixing all of them and perhaps add a validator unit test that looks for this very common mistake.
This PR fixes a couple of logical errors and cleans up some other components as well as improves the coverage of some classes. See the change comments below about the relevant details.