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 Proxy domain to use timedCancellable #464

Closed
tegefaulkes opened this issue Sep 29, 2022 · 8 comments · Fixed by #468
Closed

Update Proxy domain to use timedCancellable #464

tegefaulkes opened this issue Sep 29, 2022 · 8 comments · Fixed by #468
Assignees
Labels
development Standard development r&d:polykey:supporting activity Supporting core activity

Comments

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Sep 29, 2022

Specification

The Proxy domain needs to be updated to use timedCancellable and better support cancellability. This means that any methods that create connections needs to support cancellation. Robust testing needs to be added to test if cancelled connections are gracefully handled from both the forward and reverse side of the connection.

Additional context

Tasks

  1. Where applicable the Proxy methods needs to support cancellation via the abort signal
  2. Where applicable the Proxy methods need to be updated to use the timedCancellabe decorator.
  3. Proxy tests need to be expanded to test a wide range of cancellation and connection failure conditions.
  4. Update punch interval to 50 ms
@tegefaulkes tegefaulkes added the development Standard development label Sep 29, 2022
@tegefaulkes tegefaulkes self-assigned this Sep 29, 2022
@tegefaulkes
Copy link
Contributor Author

Looks ike the proxy domain is using a object map for tracking locking. We can switch this over to a LockBox.

@tegefaulkes
Copy link
Contributor Author

The Connection constructor takes some parameters for configuration of the connection.

    keepAliveTimeoutTime = 20000,
    endTime = 1000,
    punchIntervalTime = 1000,
    keepAliveIntervalTime = 1000,

These should be configurable from the config file.

@CMCDragonkai
Copy link
Member

Don't directly import them from the config. I believe these are meant to be injected by PolykeyAgent. In fact they should be done directly by injecting config into PolykeyAgent construction or during start. Which is only done by the CLI commands that reference the config.ts.

@CMCDragonkai
Copy link
Member

It was either that or that PolykeyAgent.ts directly references config.ts.

@CMCDragonkai
Copy link
Member

Although, I do use some constants from config.ts inside the keys domain that aren't supposed to change...

@CMCDragonkai
Copy link
Member

Ok actually I remember now. The config.ts is supposed to be constants that don't actually change. Or only change with the source code.

Parameters that are supposed to change due to runtime conditions are not supposed to be in config.ts, and instead are passed in directly from CLI/env variables and then to do the constructor and start parameters.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Oct 5, 2022

So implementation is done by we have 1 more test to do here:

  1. It is to address Integrate TaskManager into NodeGraph and Discovery #445 (comment) discovered error being reported during a lower timeout for pinging a node. We don't know whether this was the pinging node, or the pinged node. We don't know if this was during the punch protocol or the keep alive protocol. We don't know if this is also stemming from client to proxy or proxy to server or proxy to proxy.

So at the very least 1 day for the reviewing everything, and 1 day to fix the above bug.

@CMCDragonkai
Copy link
Member

@tegefaulkes make sure to update the spec with what has changed in the interface. That they all take Timer object, and any other changes to the behaviour.

@CMCDragonkai CMCDragonkai added the r&d:polykey:supporting activity Supporting core activity 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:supporting activity Supporting core activity
Development

Successfully merging a pull request may close this issue.

2 participants