rewrite OperationRetry and add unit test to address issue #879 #882
Conversation
|
RxJava-pull-requests #814 SUCCESS |
| public void onCompleted() { | ||
| observer.onCompleted(); | ||
| } | ||
| private void subscribeToSource() { |
There was a problem hiding this comment.
You need to perform the subscription on a scheduler such as currentThread to avoid stack overflow.
In addition,
- it is not advised to call subscribe from a constructor,
- the call from the constructor is not protected by a lock and there is a race condition between
subscribed.setandunsubscribeFromSource, - even if using lock,
subscribeToSourcemay be reentered before the subscribe returns andsubscribedmight not be set to true at all, - you'd need to use MultipleAssignmentSubscription which does the correct lifecycle management for this operator (the original bug was a per-source shared composite instead of a per-observer composite plus the retry counter).
There was a problem hiding this comment.
Thanks very much for the detailed feedback. I can see your points. Would
you like me to try again or just leave it to you experts?
On 17 Feb 2014 17:34, "akarnokd" [email protected] wrote:
In rxjava-core/src/main/java/rx/operators/OperationRetry.java:
@Overridepublic void onCompleted() {observer.onCompleted();}private void subscribeToSource() {You need to perform the subscription on a scheduler such as currentThread
to avoid stack overflow.This is an excellent point, I'll remember this one!
In addition,
- it is not advised to call subscribe from a constructor,
ok
- the call from the constructor is not protected by a lock and there
is a race condition between subscribed.set and unsubsctibeFromSource,no race condition because no one can unsubscribe till the
RetrySubscription is constructed. If the sub was moved to a method called
after construction then would use lock.
even if using lock, subscribeToSource may be reentered before the
subscribe returns and subscribed might not be set to true at all,ah yep, indeed
- you'd need to use MultipleAssignmentSubscription which does the
correct lifecycle management for this operator (the original bug was a
per-source shared composite instead of a per-observer composite plus the
retry counter).I'll have a look at that. Was a good exercise for me, sounds like the
original source (pre my version) may just need a slight adjustment in line
with your description of the bug. I'll have a look at it anyway out of
interest. Have you got a fix lined up already? Thanks for your time with my
code!
DaveI
Reply to this email directly or view it on GitHubhttps://github.com//pull/882/files#r9783144
.
There was a problem hiding this comment.
I just posted my rewrite for 0.17+ in #884. I don't know what the policy is about bugs in 0.16.x as 0.17 is about to ship. If you want, you could rework your fix.
There was a problem hiding this comment.
Just saw your commit. If that is the version to ship that's the one that
matters. Thanks for addressing the issue.
Dave
On 17 Feb 2014 19:34, "akarnokd" [email protected] wrote:
In rxjava-core/src/main/java/rx/operators/OperationRetry.java:
@Overridepublic void onCompleted() {observer.onCompleted();}private void subscribeToSource() {I just posted my rewrite for 0.17+ in #884#884.
I don't know what the policy is about bugs in 0.16.x as 0.17 is about to
ship. If you want, you could rework your fix.Reply to this email directly or view it on GitHubhttps://github.com//pull/882/files#r9784759
.
…or to use per observer composite subscription and attempts count
|
RxJava-pull-requests #816 SUCCESS |
|
RxJava-pull-requests #818 SUCCESS |
|
Fixed in master: #960 |
I rewrote the OperationRetry class from first principles if you like. Pretty clear to my eyes and doesn't use a single CompositeSubscription (method add()) for the lifetime of the retry which was the cause of bug #879 in the first place. Unit tests pass though I'll be glad of some peer review.
Cheers. Dave