-
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: fix Single.timeout unnecessary dispose calls #5586
Conversation
Codecov Report
@@ Coverage Diff @@
## 2.x #5586 +/- ##
============================================
+ Coverage 96.15% 96.23% +0.08%
- Complexity 5827 5828 +1
============================================
Files 632 632
Lines 41459 41467 +8
Branches 5742 5745 +3
============================================
+ Hits 39863 39904 +41
+ Misses 638 618 -20
+ Partials 958 945 -13
Continue to review full report at Codecov.
|
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.
Mostly LGTM, but few comments
if (d != DisposableHelper.DISPOSED && compareAndSet(d, DisposableHelper.DISPOSED)) { | ||
DisposableHelper.dispose(task); | ||
actual.onError(e); | ||
} |
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.
Shouldn't we deliver error to RxJavaPlugins.onError()
in else
block?
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.
Indeed. Fixed.
if (other == null) { | ||
actual.onError(new TimeoutException()); | ||
} else { | ||
this.other = null; |
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.
Hmm, are you trying to avoid memory leak here? Reference to other is still held as a final field in SingleTimeout
I mean, final reference to other
in TimeoutMainObserver
will make code slightly more readable
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.
The outer class has to reference other because of multiple subscribers. The inner observer has no use to the other once it has switched to it. There was an issue that some time ago that other was some cached resource that didn't go away when the user wanted it. This will somewhat help in that situation.
|
||
@Test | ||
public void normalSuccessDoesntDisposeMain() { | ||
final int[] calls = { 0 }; |
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: I know you often use arrays for that, but AtomicInteger
kinda much more readable when you need a reference to single "mutable" int :)
This PR fixes the unnecessary
dispose
call towards the main source when the source terminates and reworks the internals to use less allocation and indirection.