Skip to content

feat: add scheduler support for timeout management in Multiselect#312

Merged
kamilsa merged 2 commits into
masterfrom
fix/multiselect-timeout
Jun 11, 2025
Merged

feat: add scheduler support for timeout management in Multiselect#312
kamilsa merged 2 commits into
masterfrom
fix/multiselect-timeout

Conversation

@kamilsa
Copy link
Copy Markdown
Contributor

@kamilsa kamilsa commented Jun 11, 2025

Adds timeout on MultiselectInstance's selectOneOf

@kamilsa kamilsa requested a review from Copilot June 11, 2025 10:59
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds scheduler support for timeout management in the Multiselect module to ensure that protocol negotiations do not hang indefinitely.

  • Introduces a timeout for negotiations in MultiselectInstance using a scheduler.
  • Updates the Multiselect and header files to pass and store the scheduler pointer.
  • Adds an onTimeout handler that closes the connection upon timeout.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/protocol_muxer/multiselect/multiselect_instance.cpp Adds scheduler-based timeout scheduling and corresponding cancellation in close().
src/protocol_muxer/multiselect.cpp Passes scheduler pointer when creating a new MultiselectInstance.
include/libp2p/protocol_muxer/multiselect/multiselect_instance.hpp Declares the scheduler member, timeout handle, and onTimeout() method.
include/libp2p/protocol_muxer/multiselect.hpp Introduces the scheduler pointer as part of the Multiselect class.

@kamilsa kamilsa requested a review from turuslan June 11, 2025 11:42
closed_ = true;

// Cancel timeout if it's still active
timeout_handle_.reset();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we reset() connection/stream, to cancel pending read/write?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it will be automatically reset, once we erase this active instance:

void Multiselect::instanceClosed(Instance instance,
const ProtocolHandlerFunc &cb,
outcome::result<peer::ProtocolName> result) {
active_instances_.erase(instance);
if (cache_.size() < kMaxCacheSize) {
cache_.emplace_back(std::move(instance));
}
cb(std::move(result));
}

It might live a little longer in the cache, but eventually a new connection will replace the one from the instance

@kamilsa kamilsa merged commit c03bf7c into master Jun 11, 2025
3 of 4 checks passed
@kamilsa kamilsa deleted the fix/multiselect-timeout branch June 11, 2025 13:16
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.

3 participants