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

Creating toObservable for Future #109

Merged
merged 4 commits into from
Jan 31, 2013
Merged

Conversation

abersnaze
Copy link
Contributor

simple wrapper that turns a future into a observable.

@benjchristensen
Copy link
Member

This code looks fine, though I'm curious about the implication of it blocking on the Future.get().

It's obviously the only way to do it without spawning another thread that blocks on that future which would take us into this conversation: #19

Is this misleading to people to think it would be asynchronous?

@Override
public Subscription call(Observer<T> observer) {
try {
T value = that.get();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should add an overload that allows passing in the timeout values?

@benjchristensen
Copy link
Member

I've added comments inline in the code as well

@benjchristensen
Copy link
Member

Looks good.

benjchristensen added a commit that referenced this pull request Jan 31, 2013
Creating toObservable for Future
@benjchristensen benjchristensen merged commit ca75023 into ReactiveX:master Jan 31, 2013
rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014
Creating toObservable for Future
jihoonson pushed a commit to jihoonson/RxJava that referenced this pull request Mar 6, 2020
jihoonson pushed a commit to jihoonson/RxJava that referenced this pull request Mar 6, 2020
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.

2 participants