-
Notifications
You must be signed in to change notification settings - Fork 7.6k
1.x: fix unsubscription and producer issues in sample(other) #3658
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
Conversation
a9ced56
to
1a1ba81
Compare
} | ||
|
||
@Override | ||
public void onCompleted() { | ||
// no need to null check, main is assigned before any of the two gets subscribed | ||
main.get().unsubscribe(); |
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.
Why change the order of unsubscribe
and onCompleted
? Is it possible that s.onCompleted
uses some resource in main
and that will be destroyed in unsubscribe
?
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 main and child are on different subscription container so child can't use anything from main. But if you think otherwise, please write a unit test.
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 child subscriber may store some resource coming from main
and use it in onCompleted
, but main
will close it in its subscription. E.g.,
@Test
public void foo() {
Observable<InputStream> source = Observable.create(new OnSubscribe<InputStream>() {
@Override
public void call(Subscriber<? super InputStream> subscriber) {
try {
final InputStream input = new URL("http://www.google.com").openStream();
subscriber.add(Subscriptions.create(new Action0() {
@Override
public void call() {
try {
input.close();
} catch (IOException e) {
e.printStackTrace();
}
}
}));
Thread.sleep(1000);
subscriber.onNext(input);
Thread.sleep(1000);
subscriber.onCompleted();
} catch (Exception e) {
subscriber.onError(e);
}
}
});
source.sample(Observable.interval(1500, 100, TimeUnit.MILLISECONDS).take(2)).subscribe(new Subscriber<InputStream>() {
private InputStream lastInputStream;
@Override
public void onCompleted() {
if (lastInputStream != null) {
BufferedReader reader = new BufferedReader(new InputStreamReader(lastInputStream));
String line = null;
try {
while ((line = reader.readLine()) != null) {
System.out.println(line);
}
} catch (IOException e) {
e.printStackTrace();
}
}
}
@Override
public void onError(Throwable e) {
e.printStackTrace();
}
@Override
public void onNext(InputStream inputStream) {
lastInputStream = inputStream;
}
});
}
1a1ba81
to
4369e1c
Compare
Restored original completion-unsubscription order + verification |
👍 |
1 similar comment
👍 |
1.x: fix unsubscription and producer issues in sample(other)
This PR fixes 2 bugs with
sample
In addition, #3657 wants to emit the very last item on completion to which I marked the required changes in comments (to be uncommented in a separate PR if needed).