-
Notifications
You must be signed in to change notification settings - Fork 114
Introduce next_event_async allowing to poll event queue
#224
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
9e7c2b6 to
c7ac3cd
Compare
| /// Will asynchronously poll the event queue until the next event is ready. | ||
| /// | ||
| /// **Note:** this will always return the same event until handling is confirmed via [`Node::event_handled`]. | ||
| pub async fn next_event_async(&self) -> Event { |
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.
its worth following the naming convention with wait_next_event and call this async_next_event
also, Its a bit confusing to have wait and async as usually they both mean async something..
maybe wait_next_event should be sync_next_event?
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.
Mh, I'm not sure: wait_next_event is called that way to follow std::sync::Condvar's naming that indicates it's going to block the current thread. I disagree that wait and async "both mean async something" as blocking or not blocking the thread is a fundamental difference here.
That said, I'm generally also not the biggest fan of the _async suffix here as it's redundant to the actual return type/async keyword of the method. I considered poll_next_event as an alternative name for next_event_async, however, it may also be a bit misleading as the semantics of Future's poll are slightly different. As we also use the _async suffix for LDK's process_events_async I stuck with that for now. Generally I'm still open for better suggestions though, poll_next_event might be an alternative candidate.
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.
Thanks for the explanation.
I would also go with poll_ , would look better than async fn name_async().
future_next_event could be another option..
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 don't mind next_event_async, would also consider next_event_future.
9075a24 to
cf06e9c
Compare
|
Rebased on #230. |
48c161c to
c43fb01
Compare
c43fb01 to
290a543
Compare
|
Rebased on main after #230 landed. |
We implement a way to asynchronously poll the queue for new events, providing an async alternative to `wait_next_event`.
.. which requires us to include a dependency on the `kotlinx-coroutines` package.
290a543 to
77dfa83
Compare
| /// Will asynchronously poll the event queue until the next event is ready. | ||
| /// | ||
| /// **Note:** this will always return the same event until handling is confirmed via [`Node::event_handled`]. | ||
| pub async fn next_event_async(&self) -> Event { |
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 don't mind next_event_async, would also consider next_event_future.
|
Going ahead with this for now, can always revisit the naming in the future (no pun intended). |
We implement a way to asynchronously poll the queue for new events, providing an async alternative to
wait_next_event.Bindings exposure is still blocked on the next UniFFI release.Now also exposed in bindings as UniFFI 0.26 has been released, now based on #230.