Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 97
Flowables::fromGenerator operator#419
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
843b548390f25350f0079ae9c9501ef6b18ea382de3f77bd4cf104214d392e7144a1d26a50d72a664caa3bcd98e14489d486775cdFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -108,6 +108,25 @@ class Flowables { | ||
| return Flowable<T>::create(std::move(lambda)); | ||
| } | ||
| template <typename T, typename TGenerator> | ||
| static Reference<Flowable<T>> fromGenerator(TGenerator generator) { | ||
| auto lambda = [generator = std::move(generator)] | ||
| (Subscriber<T>& subscriber, int64_t requested) { | ||
| int64_t generated = 0; | ||
| try { | ||
| while (generated < requested) { | ||
| subscriber.onNext(generator()); | ||
| ++generated; | ||
| } | ||
| return std::make_tuple(static_cast<int64_t>(generated), false); | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this isn't related to this diff, but I find this API weird, why does it have to return how many onNext it produced and if it was completed? both of those could be inferred by passing a Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was written by @vjn. It's how ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, we discussed that. I asked the same question originally. There was a push back on creating a wrapper around every subscriber to keep track of the count. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like the wrapper would result in the exact same code after optimization (it would be templated on the lambda). This API makes it easy to fuck up by either returning the wrong number (off-by-one seems easy) or | ||
| } catch(const std::exception&) { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should also have a case for | ||
| subscriber.onError(std::current_exception()); | ||
| return std::make_tuple(static_cast<int64_t>(generated), true); | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
| ||
| } | ||
| }; | ||
| return Flowable<T>::create(std::move(lambda)); | ||
| } | ||
| private: | ||
| Flowables() = delete; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -51,6 +51,8 @@ class FlowableOperator : public Flowable<D> { | ||
| subscriber_->onSubscribe(Reference<::yarpl::flowable::Subscription>(this)); | ||
| } | ||
| void onNext(U) override {} | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you think | ||
| void onComplete() override { | ||
| subscriber_->onComplete(); | ||
| upstream_.reset(); | ||
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.
if the subscriber calls cancel() in the onNext() won't this still deliver another one?
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.
@alexmalyshev Should I just emit one value at a time and let Flowable go thru the lambda multiple times? Flowables::range has the same problem. We can generate values even after cancel.
Technically speaking that is ok for the reactive streams protocol. It says after cancel we may receive some more values...
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.
I think we should leave this as is. The spec has no guarantees about synchronizing
onNext()withcancel(), and I don't think there's a great solution that fits all client use-cases.