p2p: fix node abort when a request races connection shutdown

[redsh4de]

What

Fixes a panic that aborts the node when a request is sent as its connection is shutting down

thread 'cuprated-tokio' (38638) panicked at helper/src/asynch.rs:33:28:
Oneshot must not be cancelled before response!: Canceled
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Aborted

Why

Client::poll_ready reserves an mpsc permit, and outstanding permits can still send after Receiver::close.

Connection::shutdown drains with try_recv, which can't wait for them, so a request that races shutdown is stranded and its oneshot::Sender dropped - panicking InfallibleOneshotReceiver.

From Receiver::close docs:

Any outstanding Permit values will still be able to send messages.

To guarantee that no messages are dropped, after calling close(), recv() must be called until None is returned. If there are outstanding Permit or OwnedPermit values, the recv method will not return None until those are released.

Where

p2p/p2p-core

How

• Drain Connection::shutdown with recv() until None, as recommended. It is tokio::spawned so that the connection task can still exit right away
• Client::poll_ready now checks if the connection is closed before anything else. It used to only find out by trying to reserve a channel slot, which always succeeds when a slot is already reserved, even after the peer is gone

[redsh4de]

Running this for about a week, no more panics on my end

[Boog900]

I haven't approved and merged this yet as I want to add a way to disarm the client service so it doesn't keep holding permits/locks if it isn't going to be called after poll_ready. I think we'll need to add an extension trait to tower::Service for this though to allow it to be used generically. I will get round to doing this before the next release unless you have a good idea on how this could be done.

[redsh4de]

This change avoids reserving permits in poll_ready in client service. Capacity is acquired inside the request future, so cancellation before enqueueing releases it automatically. A disarm trait would need callers remembering to use it, which is akin to the malloc/free footgun imo

[Boog900]

That removes the need for the semaphore completely and stops people from being able to make decisions on readiness. I'll have to think about it.