Skip to content

Use unbounded channels to avoid dropped requests#335

Merged
rustaceanrob merged 1 commit intomasterfrom
client-channel-4-17
Apr 17, 2025
Merged

Use unbounded channels to avoid dropped requests#335
rustaceanrob merged 1 commit intomasterfrom
client-channel-4-17

Conversation

@rustaceanrob
Copy link
Copy Markdown
Collaborator

The UnboundedSender offers two advantages over bounding the number of requests a client can make. First it drops the need to maintain an async and sync version of the client methods - UnboundedSender::send is synchronous. This is actually very convient for ldk-node, where the client will have to implement a synchronous trait.

Secondly there are some methods that are critical to succeed if the node is running. Adding a script must always work if the node is still online. Because it is unacceptable to miss some messages, there is basically two approaches: cache the failures and try again later, or use an unbounded channel. Since caching the failures will result in the exact same overhead as stored messages in a channel when the node is not ready to accept them yet, the unbounded channel is much easier to implement and reason about.

The `UnboundedSender` offers two advantages over bounding the number
of requests a client can make. First it drops the need to maintain an
`async` and sync version of the client methods - `UnboundedSender::send`
is synchronous. This is actually very convient for `ldk-node`, where
the client will have to implement a synchronous trait.

Secondly there are some methods that are critical to succeed if the node
is running. Adding a script must always work if the node is still online.
Because it is unacceptable to miss some messages, there is basically two
approaches: cache the failures and try again later, or use an unbounded
channel. Since caching the failures will result in the exact same overhead
as stored messages in a channel when the node is not ready to accept them
yet, the unbounded channel is much easier to implement and reason about.
@nyonson
Copy link
Copy Markdown
Collaborator

nyonson commented Apr 17, 2025

This interface looks good to me, so first point makes sense. But I'm not following the second. Does this change make anything more reliable? Seems like it just removes back pressure (which seems fine in this scenario).

@rustaceanrob
Copy link
Copy Markdown
Collaborator Author

rustaceanrob commented Apr 17, 2025

I incorrectly recalled this as a problem for the mpsc receivers but it is actually a broadcast::Receiver problem. Either way the message just fails on the send side for bounded channels, which still results in this problem of caching failures. Instead of doing our own retry logic I think using the built-in is better here.

@nyonson
Copy link
Copy Markdown
Collaborator

nyonson commented Apr 17, 2025

Aren't bounded and unbounded exposed to the same types of send failures? Sorry being slow here, just feels like I am missing something. I thought bounded are made async in case the buffer is full and it can send back pressure to the caller. And unbounded can still fail in some catastrophic way if memory runs out. But neither fail due to work done on the receiving side right?

Copy link
Copy Markdown
Collaborator

@nyonson nyonson left a comment

Choose a reason for hiding this comment

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

ACK 053261d

@rustaceanrob
Copy link
Copy Markdown
Collaborator Author

If a send to an unbounded channel fails then the channel is either closed or the receiver has been dropped. In both cases that would mean the node has stopped. In the bounded case you'll notice the Sender::send error variant has a T with it, which is intended to be used if the queue is currently full. In the bounded case the send can fail if the channel is closed/dropped, but can also fail if the queue is full and the node is currently processing a request.

@nyonson
Copy link
Copy Markdown
Collaborator

nyonson commented Apr 17, 2025

In the bounded case the send can fail if the channel is closed/dropped, but can also fail if the queue is full and the node is currently processing a request.

Ok, that mostly checks out with my understanding. But I don't think Sender::send fails if the queue is full, I think it just suspends?

@rustaceanrob
Copy link
Copy Markdown
Collaborator Author

In short, yes you're right, but I'm going to ignore the catastrophic failure case here because the requests are very small, either scripts or transactions most of the time

@rustaceanrob rustaceanrob merged commit f21e475 into master Apr 17, 2025
10 checks passed
@rustaceanrob rustaceanrob deleted the client-channel-4-17 branch April 17, 2025 16:21
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