fix: optimize compute frequency of the locally supported protocols
Description
https://github.com/libp2p/rust-libp2p/issues/4284
Minimise the runtime impact of this function by calling not more than once every 5 seconds.
Also fixes the case where an idle connection would not update the local supported protocols after retunring Poll::Pending.
Notes & open questions
I can see several tests failing. I assume they fail because of timing issues that have been introduced by this 5 second interval.
Any help or ideas debugging them would be great, as I'm not familiar with them.
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
Note that the 500ms interval is for the tests, but I'm curious how we can properly estimate those.
I think the issue with this approach is that even a completely idle connection will now wake up every 500ms to update its supported protocols, despite there being no changes, correct?
yes. but the only way for the local protocols to change is if Connection::poll is called and one of the other futures makes progress?
Note that the 500ms interval is for the tests, but I'm curious how we can properly estimate those.
I think the issue with this approach is that even a completely idle connection will now wake up every 500ms to update its supported protocols, despite there being no changes, correct?
yes. but the only way for the local protocols to change is if
Connection::pollis called and one of the other futures makes progress?
Yes. Unless I am missing something, the version in this PR will continuously wake the Connection because the timer fires, even if the protocols hadn't changed in-between. In other words, I think the timer should fire only once for a Connection that returned Poll::Pending and even then only if the set of protocols is potentially outdated because we have invoked the ConnectionHandler since we last computed it.
This pull request has merge conflicts. Could you please resolve them @alindima? 🙏