Skip to content

Introduce next_event_async allowing to poll event queue - #224

Merged
tnull merged 3 commits into
lightningdevkit:mainfrom
tnull:2024-01-async-events
Feb 19, 2024
Merged

Introduce next_event_async allowing to poll event queue#224
tnull merged 3 commits into
lightningdevkit:mainfrom
tnull:2024-01-async-events

Conversation

@tnull

@tnulltnull commented Jan 3, 2024

Copy link
Copy Markdown
Collaborator

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.

@tnull
tnullforce-pushed the 2024-01-async-events branch 3 times, most recently from 9e7c2b6 to c7ac3cdCompareJanuary 4, 2024 13:16
Comment threadsrc/lib.rs
/// 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 {

Copy link
Copy Markdown
Contributor

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?

@tnulltnullJan 10, 2024

Copy link
Copy Markdown
CollaboratorAuthor

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.

Copy link
Copy Markdown
Contributor

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..

Copy link
Copy Markdown
Contributor

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.

@tnull
tnullforce-pushed the 2024-01-async-events branch 2 times, most recently from 9075a24 to cf06e9cCompareJanuary 24, 2024 09:50
@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Rebased on #230.

@tnull
tnullforce-pushed the 2024-01-async-events branch 6 times, most recently from 48c161c to c43fb01CompareJanuary 24, 2024 13:20
@tnull
tnullforce-pushed the 2024-01-async-events branch from c43fb01 to 290a543CompareFebruary 2, 2024 12:21
@tnull

tnull commented Feb 2, 2024

Copy link
Copy Markdown
CollaboratorAuthor

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.
@tnull
tnullforce-pushed the 2024-01-async-events branch from 290a543 to 77dfa83CompareFebruary 16, 2024 14:22
@tnull
tnull requested a review from wpaulinoFebruary 16, 2024 14:22
Comment threadsrc/lib.rs
/// 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 {

Copy link
Copy Markdown
Contributor

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.

@tnull

Copy link
Copy Markdown
CollaboratorAuthor

Going ahead with this for now, can always revisit the naming in the future (no pun intended).

@tnull
tnull merged commit 769a2d2 into lightningdevkit:mainFeb 19, 2024
@tnulltnull mentioned this pull request Feb 19, 2024
19 tasks
@tnulltnull added this to the 0.3 milestone Feb 19, 2024
Sign up for freeto 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.

3 participants

@tnull@wpaulino@jbesraa