-
Notifications
You must be signed in to change notification settings - Fork 7.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
2.x: improve Flowable.timeout() #5661
Conversation
Codecov Report
@@ Coverage Diff @@
## 2.x #5661 +/- ##
============================================
- Coverage 96.24% 96.21% -0.04%
+ Complexity 5853 5818 -35
============================================
Files 635 633 -2
Lines 41651 41553 -98
Branches 5768 5752 -16
============================================
- Hits 40089 39982 -107
- Misses 616 621 +5
- Partials 946 950 +4
Continue to review full report at Codecov.
|
|
||
actual.onNext(t); | ||
long idx = get(); | ||
if (idx == Long.MAX_VALUE || !compareAndSet(idx, idx + 1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe leave a link to your blog post in comment? So readers could understand the pattern used here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really keen on this. The PR will reference the blog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, my point is not to promote your blog haha
Just want to leave some bread crumbs in the code so one could understand that MAX_VALUE
is used as a notion of terminal state here, one might confuse it with something related to unbounded request mode
I'd just put
@see <a href="https://akarnokd.blogspot.hu/2017/09/java-9-flow-api-timing-out-events.html">Operator Design Principles</a>
To the class's javadoc.
It's obviously a nit and up2u :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, but I won't put it there.
@Override | ||
public void onNext(Object t) { | ||
if (get() != SubscriptionHelper.CANCELLED) { | ||
get().cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract get()
result to local var here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
This PR improves the internal overhead of the
Flowable.timeout
operator (its 2 timed and 2 selector-based versions) to use the adapted indexed atomic state transition approach.In addition, there was a race condition and potential event loss in
TestScheduler
:peek()
could returnnull
if the task was removed from the queue (via disposing it) between theisEmpty()
check andpeek()
itself.peek
andremove
, theremove
could remove the new head of the queue which is no longer whatpeek
saw and results in tasks being dropped.