Skip to content
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

isolate subscriber used for retries, cleanup tests #1027

Merged
merged 1 commit into from
Apr 12, 2014

Conversation

petermd
Copy link
Contributor

@petermd petermd commented Apr 9, 2014

potential fix for #1024

there is an outstanding question (see commit comments) about whether its appropriate for the operator to use unsafeSubscribe given that it does not completely isolate the Subscriber from a badly behaved Observable.

@cloudbees-pull-request-builder

RxJava-pull-requests #946 FAILURE
Looks like there's a problem with this pull request

@zsxwing
Copy link
Member

zsxwing commented Apr 10, 2014

@benjchristensen can you clarify the usage of unsafeSubscribe? I'm also confused now. Thank you.

@benjchristensen
Copy link
Member

It wouldn't have been getting SafeSubscriber before unsafeSubscribe anyways because this operator lives inside rx.operator so the wrapping would have been skipped.

It would have had some extra try/catch error handling in synchronous execution prior to the unsafeSubscribe change, but that should be it.

The use of unsafeSubscribe leaves operators to ensure contracts are kept. The SafeSubscriber should really only be used to wrap the user-provided Subscriber or Observer at the very end because it has uber-error-handling, calls execution hooks, etc.

Some operators will definitely need to do special things like unsubscribing, such as merge and retry – but those would not have ever been happening since isInternalImplementation checks in Observable would have prevented these from being wrapped by SafeSubscriber before, so any bugs found about this should have already existed.

Operators should just propagate errors or let them throw since the final SafeSubscriber or lift will handle them, which is why unsafeSubscribe is as lightweight as it is.

I'll review this code and merge or modify if needed.

benjchristensen added a commit to benjchristensen/RxJava that referenced this pull request Apr 12, 2014
benjchristensen added a commit that referenced this pull request Apr 12, 2014
@benjchristensen benjchristensen merged commit d9fef71 into ReactiveX:master Apr 12, 2014
@benjchristensen
Copy link
Member

The fix is good, thank you @petermd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants