-
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
rx.Single #3012
rx.Single #3012
Conversation
I don't see any way to cancel a subscription to a |
When using The We don't currently have something like The What do you think should be done? |
but This throws NPE: Single.create(s -> {
new Thread(() -> {
try {
Thread.sleep(5000);
} catch (Exception e) {
e.printStackTrace();
}
// deliver value
s.onSuccess("Data=" + arg);
}).start();
}).timeout(1, TimeUnit.SECONDS).subscribe(System.out::println, Throwable::printStackTrace);
Thread.sleep(2000);
Exception in thread "main" java.lang.NullPointerException
at rx.Single.toObservable(Single.java:232)
at rx.Single.timeout(Single.java:1817)
at rx.Single.timeout(Single.java:1740)
at SingleTest.main(SingleTest.java:36) If I use the other overload of timeout, it quits only after 5 seconds, instead of 2: public static void main(String[] args) throws Exception {
Single.create(s -> {
new Thread(() -> {
try {
Thread.sleep(5000);
} catch (Exception e) {
e.printStackTrace();
}
// deliver value
s.onSuccess("Data");
}).start();
}).timeout(1, TimeUnit.SECONDS, Single.just("Alt"))
.subscribe(System.out::println, Throwable::printStackTrace);
Thread.sleep(2000);
} |
See what I said about that in the previous comment. In 2.x we intend on not returning |
49cd562
to
56eb890
Compare
Yup, it certainly did :-) Unit test added. I had a null not being handled. |
56eb890
to
4394e76
Compare
These tests are passing and not taking 5 seconds: @Test
public void testTimeout() {
TestSubscriber<String> ts = new TestSubscriber<String>();
Single<String> s = Single.create(new OnSubscribe<String>() {
@Override
public void call(SingleObserver<? super String> s) {
try {
Thread.sleep(5000);
} catch (InterruptedException e) {
// ignore as we expect this for the test
}
s.onSuccess("success");
}
}).subscribeOn(Schedulers.io());
s.timeout(100, TimeUnit.MILLISECONDS).subscribe(ts);
ts.awaitTerminalEvent();
ts.assertError(TimeoutException.class);
}
@Test
public void testTimeoutWithFallback() {
TestSubscriber<String> ts = new TestSubscriber<String>();
Single<String> s = Single.create(new OnSubscribe<String>() {
@Override
public void call(SingleObserver<? super String> s) {
try {
Thread.sleep(5000);
} catch (InterruptedException e) {
// ignore as we expect this for the test
}
s.onSuccess("success");
}
}).subscribeOn(Schedulers.io());
s.timeout(100, TimeUnit.MILLISECONDS, Single.just("hello")).subscribe(ts);
ts.awaitTerminalEvent();
ts.assertNoErrors();
ts.assertValue("hello");
} |
With code in its current shape, intended for experimentation, and avoiding any controversial public APIs, shall we proceed with merging it? |
Ship that puppy. |
No backpressure? |
Saved by the interrupt. But what if Single isn't subscribed to on one of the standard Schedulers, or it is subscribed to from a scheduler which doesn't call Future.cancel() with true? Besides, the programming advantage of Subscriber is that one can add resources to it which will get unsubscribed automatically. So instead of SingleObserver, I suggest the following interface: interface SingleSubscriber<T> {
void onSubscribe(Subscription s);
void onSuccess(T value);
void onError(Throwable ex);
}
Single.create(s -> {
Thread t = new Thread(() -> {
try {
Thread.sleep(5000);
} catch (Exception e) {
e.printStackTrace();
}
// deliver value
s.onSuccess("Data");
});
s.onSubscribe(Subscription.create(() -> t.interrupt());
t.start();
})
Single.create(s -> {
SubscriptionList slist = new SubscriptionList();
s.onSubscribe(slist);
try (InputStream in = new FileInputStream("file.dat")) {
slist.add(Subscriptions.create(() -> Closeables.closeSilently(io)));
byte[] data = new byte[in.available()];
in.read(data);
s.onSuccess(data);
} catch (IOException ex) {
if (!slist.isUnsubscribed()) {
s.onError(ex);
}
}
}); |
private static <T> Observable<T> toObservable(Single<T> t) { | ||
// is this sufficient, or do I need to keep the outer Single and subscribe to it? | ||
return Observable.create(t.onSubscribe); | ||
} |
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 isn't this a public instance method? I'd like to write someSingle.toObservable()
.
And symmetrically, there could be a toSingle()
method in Observable
.
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.
Only because I'm being cautious in what I add to the public API. We can always add, but we can't take things away.
This static method exists only because I'm using it to implement the various operators.
I'm open to adding an instance toObservable()
method.
Interesting. I'm not thrilled though by the implicit contract this creates unless invoking If we're going this route we should just skip the I'll make changes to accommodate this and ask for your feedback. |
For a single valued emission, no I don't think it should care. It significantly complicates things and I think it's silly to worry about backpressure and flow control on a type that emits a single notification, either When it gets composed however, then the backpressure semantics of |
I guess @davidmoten meant where the Single and Observable meet, for example in toObservable() where you should use |
It follows the reactive-streams concept. Without it, the cancellation wouldn't compose through and you'd rely only upon the ability of |
4394e76
to
66ec579
Compare
I know, but I don't like mixing patterns in v1. It would be odd to have Take a look at the code now. It has a |
Ah okay, I can buy into that ... so like this: protected Single(final OnSubscribe<T> f) {
// bridge between OnSubscribe (which all Operators and Observables use) and OnExecute (for Single)
this.onSubscribe = new Observable.OnSubscribe<T>() {
@Override
public void call(final Subscriber<? super T> child) {
final SingleDelayedProducer<T> producer = new SingleDelayedProducer<T>(child);
child.setProducer(producer);
SingleSubscriber<T> ss = new SingleSubscriber<T>() {
@Override
public void onSuccess(T value) {
producer.setValue(value);
}
@Override
public void onError(Throwable error) {
child.onError(error);
}
};
child.add(ss);
f.call(ss);
}
};
} It still feels like a lot of overhead for something that should never need backpressure :-/ If this shows poor performance I would drop this and just leave it to things like |
66ec579
to
7876b0c
Compare
See test for backpressure here: 7876b0c#diff-6e59701fc9262bf5107abc5d12d7a928R375 for unsubscribe: 7876b0c#diff-6e59701fc9262bf5107abc5d12d7a928R252 |
I thought its easier to change
I haven't benchmarked this yet, but I believe |
Comparing with a stream of 1000 is not a valid comparison. A The reason a Note that
But then the test cases I provided wouldn't work where a Look at what If we agree with the public API of Looking at the code as it currently stands, is there anything more you want changed before proceeding with it? |
So I guess delivering 1000 items through AbstractOnSubscribe one at a time isn't really comparable with AbstractProducer emitting the same amount when requested all at once. Regardless, If the use case for
It really depends on how long the computation takes to become available: if it is in microsecond or millisecond range, then the few dozen nanoseconds CAS takes inside the
So I'm guessing there won't be any unsubscription possibility if one uses the non- Subscriber or SingleSubscriber-based
Perhaps you could include benchmarks already so the improvements can be measured. |
Sure, but that's why we have different abstractions, types and tools.
The point of
I guess we can make those return |
7876b0c
to
88bf641
Compare
Adds `rx.Single` as an "Observable Future" for representing work with a single return value. See ReactiveX#1594 rx.Future/Task/Async/Single This provides a type similar to `Future` in that it represents a scalar unit of work, but it is lazy like an `Observable` and many `Single`s can be combined into an `Observable` stream. Note how `Single.zip` returns `Single<R>` whereas `Single.merge` returns `Observable<R>`. Examples of using this class: ```java import rx.Observable; import rx.Single; public class TaskExamples { public static void main(String... args) { // scalar synchronous value Single<String> t1 = Single.create(t -> { t.onSuccess("Hello World!"); }); // scalar synchronous value using helper method Single<Integer> t2 = Single.just(1); // synchronous error Single<String> error = Single.create(t -> { t.onError(new RuntimeException("failed!")); }); // executing t1.subscribe(System.out::println); t2.subscribe(System.out::println); error.subscribe(System.out::println, e -> System.out.println(e.getMessage())); // scalar Singles for request/response like a Future getData(1).subscribe(System.out::println); // combining Tasks into another Task Single<String> zipped = Single.zip(t1, t2, (a, b) -> a + " -- " + b); // combining Singles into an Observable stream Observable<String> merged = Single.merge(t1, t2.map(String::valueOf), getData(3)); Observable<String> mergeWith = t1.mergeWith(t2.map(String::valueOf)); zipped.subscribe(v -> System.out.println("zipped => " + v)); merged.subscribe(v -> System.out.println("merged => " + v)); mergeWith.subscribe(v -> System.out.println("mergeWith => " + v)); } /** * Example of an async scalar execution using Single.create * <p> * This shows the lazy, idiomatic approach for Rx exactly like an Observable except scalar. * * @param arg * @return */ public static Single<String> getData(int arg) { return Single.create(s -> { new Thread(() -> { try { Thread.sleep(500); } catch (Exception e) { e.printStackTrace(); } // deliver value s.onSuccess("Data=" + arg); }).start(); }); } } ```
88bf641
to
0e0949d
Compare
Perf, as expected, is poor for representing microsecond scale computations, but totally fine for IO related request/response which this is modeling:
Also note that we are maintaining the same performance hit of |
Proceeding with merge. It is all marked as |
Adds
rx.Single
as an "Observable Future" for representing work with a single return value.See #1594 rx.Future/Task/Async/Single
This provides a type similar to
Future
in that it represents a scalar unit of work, but it is lazy like anObservable
and manySingle
s can be combined into anObservable
stream. Note howSingle.zip
returnsSingle<R>
whereasSingle.merge
returnsObservable<R>
.Examples of using this class: