refactor(kad): split debug_assert

[thomaseizinger]

Description

Notes & open questions

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[thomaseizinger]

@stormshield-frb Would you mind testing bootstrapping with this branch?

[stormshield-frb]

Hi @thomaseizinger. We don't really need to test this because this is our current workaround:

if let Err(push_err) = result {
    match push_err {
        futures_bounded::PushError::BeyondCapacity(_) => {
            debug_assert!(false, "Expected to not create more streams than allowed");
        }
        futures_bounded::PushError::Replaced(_) => {
            tracing::warn!(
                "Query {:?} already has a pending stream request, replaced existing",
                id
            );
        }
    }
}

So we can confirm that we indeed go through the PushError::Replaced case. For the current time being, we did not observe any noticeable side effects.

However, since a future is replaced (ie. dropped), the receiver owned by the future is dropped :

rust-libp2p/protocols/kad/src/handler.rs

Lines 556 to 558 in 543408b

	let mut stream = receiver
	.await
	.map_err(|_| io::Error::from(io::ErrorKind::BrokenPipe))?

so, when the sender try to send the result of the new stream opening, the .send() fails (which is silently ignored) :

rust-libp2p/protocols/kad/src/handler.rs

Lines 792 to 794 in 543408b

	if let Some(sender) = self.pending_streams.pop_front() {
	let _ = sender.send(Err(ev.error));
	}

On the other end, it is legitimate to have several time the same QueryId in the same Connection at the "same" time? (in our case it is a QueryId from a bootstrap triggering HandlerIn::FindNodeReq).

Moreover, when the try_push returns PushError::Replaced, the replaced future is, in the end, stopped and dropped. Is it not a problem?

[stormshield-frb]

By the way, we noticed the same assumption (that an error is always PushError::BeyondCapacity) multiple times in a more recent work in protocols/relay/src/priv_client/handler.rs

rust-libp2p/protocols/relay/src/priv_client/handler.rs

Lines 224 to 226 in 543408b

	if result.is_err() {
	tracing::warn!("Dropping in-flight connect request because we are at capacity")
	}

[thomaseizinger]

On the other end, it is legitimate to have several time the same QueryId in the same Connection at the "same" time? (in our case it is a QueryId from a bootstrap triggering HandlerIn::FindNodeReq).

Ah this is explains it! Well, in this case, we should probably just move from FuturesMap to FuturesSet. I didn't know that QueryIds would actually not be unique.

cc @mxinden Was this on purpose? I think this is something that we should definitely document or change. It isn't intuitive at all!

This also means the current logic is completely broken because we will abort a lot of outgoing requests. I'll see to send a fix later!