Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Discovery domain to use timedCancellable #477

Closed
tegefaulkes opened this issue Oct 7, 2022 · 2 comments
Closed

Update Discovery domain to use timedCancellable #477

tegefaulkes opened this issue Oct 7, 2022 · 2 comments
Assignees
Labels
development Standard development r&d:polykey:core activity 3 Peer to Peer Federated Hierarchy

Comments

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Oct 7, 2022

Specification

The discovery domain has a long running process of connecting to other nodes. This needs to support cancellation. On review, there is only one method that needs cancellablility, that is the processVertex method. This is already used as a task handler so it already supports cancellablility. But to cap off this issue some small changes still need to be made.

Since processVertex is a compound task, it doesn't make sense to have a default timeout for it. The overall timeout of the method should be the result of the individual timeouts of the method calls within it. We still need to support an overall timeout for the whole process if we want it. this means we only pass the signal into sub calls and create new timers for each one based on the parameter.

Each downstream call that has it's own timeout needs to have it's timeout set as a parameter of the processVertex() method. The main one is the nodeManager.requestChainData() calls but there may be others that make connections to nodes. These still need to be updated with a context, I must've missed them in the nodes domain PR.

Additional context

Tasks

  1. Default the timeout for processVertex to infinity.
  2. Add a parameter for override timeout for each sub-connection. Create a new timer for each connection and propagate the signal.
  3. These connections need to take an optional ctx context, but we don't need to fully convert them to timedCancellable since only the nodeConnectionManager.withConnF makes use of the ctx and that's already a timedCancellable.
@CMCDragonkai
Copy link
Member

@tegefaulkes can you check how much of #462 is relevant to this issue? In the interests of time, we only want to ensure that don't wait 20 seconds for each discovery operation.

@tegefaulkes
Copy link
Contributor Author

At first glance, This issue is reasonably separate from #462. All I have to do to complete this issue and #353 Is to go over the timedCancellable usage, update the cancellation logic and make sure that any 'child' connection created as part of the discovery loop has it's own timeout as per #353.

#462 deals with a deeper refactor of the logic and appying new features such as the priority and TTL.

@CMCDragonkai CMCDragonkai added the r&d:polykey:core activity 3 Peer to Peer Federated Hierarchy label Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:core activity 3 Peer to Peer Federated Hierarchy
Development

No branches or pull requests

2 participants