Handeling of events received in kad::Handler could be delayed or blocked

[stormshield-frb]

Handeling of events received in kad::Handler could be delayed or blocked

[mxinden]

Thank you for your debugging work and the elaborate explanation.

I am still confused by one thing. Sorry in case I am missing something. It is rather late in my day.

Once the connection task called on_behaviour_event on the ConnectionHandler, it will go ahead and poll the ConnectionHandler, given that both is running in a loop.

rust-libp2p/swarm/src/connection/pool/task.rs

Lines 178 to 186 in 9584ee3

	loop {
	match futures::future::select(
	command_receiver.next(),
	poll_fn(|cx| Pin::new(&mut connection).poll(cx)),
	)
	.await
	{
	Either::Left((Some(command), _)) => match command {
	Command::NotifyHandler(event) => connection.on_behaviour_event(event),

Shouldn't this make the fix in #4961 obsolete?

[thomaseizinger]

Good catch @mxinden! I think you are right and #4961 should indeed not be necessary.

[stormshield-frb]

Yes indeed. I had not succeeded in finding where the Handler::poll was called and in what context, thanks for that @mxinden. If it is the only place, I also think that the bug can not happen with the current implementation.

As for #4961 not being necessary, indeed, since there does not seem to be a case where the bug could happen, it might not be required to do it. However, we should be careful in the future because, since the contract of the poll method is broken (a function returning Poll::Pending must also ensure that the current task is scheduled to be awoken when progress can be made), if a future change is made to this part of the codebase (like the change that caused #4860, for example), the bug could then be revealed.

I'll let you decide what you prefer, and I will understand either way 😊

[thomaseizinger]

I'll close this as not necessary. If you want, we can merge a documentation update that explains this @stormshield-frb ! Thank you for the work! :)