Skip to content

1.x: new fromAsync to bridge the callback world with the reactive #4179

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

Merged
merged 3 commits into from
Jul 10, 2016

Conversation

akarnokd
Copy link
Member

@akarnokd akarnokd commented Jul 8, 2016

This PR adds a new source operator: fromAsync() that let's bridge the callback-style world with the reactive world by providing a push surface and offers options to handle backpressure.

@akarnokd
Copy link
Member Author

akarnokd commented Jul 8, 2016

These errors, i.e., Travis running out of memory and killing tests, gets annoying...

}
}

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Really awesome tests. Easy to read and understand.

@akarnokd
Copy link
Member Author

akarnokd commented Jul 8, 2016

Updated.

@JakeWharton
Copy link
Contributor

LGTM 👍 !

I'm surprised you couldn't reuse more of the existing infrastructure for the various backpressure modes here and instead need to have a full implementation of the whole queue/drain stuff.

@akarnokd
Copy link
Member Author

akarnokd commented Jul 8, 2016

The codebase spans over several years now and there is no current "best toolset" for building operators. Besides, this inline saves allocation and overhead from applying other operators.

@akarnokd akarnokd added this to the 1.2 milestone Jul 8, 2016
@akarnokd
Copy link
Member Author

akarnokd commented Jul 8, 2016

/cc @stevegury @zsxwing as this is a new operator proposed to the public API


t.add(emitter);
t.setProducer(emitter);
asyncEmitter.call(emitter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't invoking this be deferred until request > 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why would we do that? The subscribe() itself triggers the execution of the body that sets up the push source.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I guess it depends on what the implementation of the emitter is. I have a few use cases in my head I'm thinking through. Besides you could use AsyncOnSubscribe if you need that request coordination.

@akarnokd
Copy link
Member Author

akarnokd commented Jul 9, 2016

@JakeWharton how eager are you about this. I'd really love to merge all remaining PRs so 1.1.7 is as complete API-vise as possible. Otherwise, we may have to wait till 1.1.8 and 1.2 is also delayed.

@JakeWharton
Copy link
Contributor

I would, of course, prefer that it made it. Releases are few and far between here so missing the boat might mean 3 months before it sees the light of day.

That said, if no one from Netflix is available to review the API and it's the only thing blocking 1.1.7 then I'm fine with it missing the boat.

@akarnokd
Copy link
Member Author

I see low risk as this is a completely new operator.

@akarnokd akarnokd merged commit 6b47b11 into ReactiveX:1.x Jul 10, 2016
@akarnokd akarnokd deleted the FromAsync branch July 10, 2016 06:36
@JakeWharton
Copy link
Contributor

Thanks!

On Sun, Jul 10, 2016, 2:36 AM David Karnok [email protected] wrote:

Merged #4179 #4179.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#4179 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAEEETB8Xs_T7eEi0pq4xF0STz3VozPxks5qUJLcgaJpZM4JIVRj
.

@defHLT
Copy link

defHLT commented Jul 12, 2016

I wonder should this method be used instead of create in cases like RxBinding present?

@stevegury
Copy link
Member

👍
Sorry for the late response (I was on vacation)

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

Successfully merging this pull request may close these issues.

4 participants