MatrixAI / Polykey

Polykey Core Library
https://polykey.com
GNU General Public License v3.0
31 stars 4 forks source link

Update `Proxy` domain to use `timedCancellable` #464

Closed tegefaulkes closed 2 years ago

tegefaulkes commented 2 years ago

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 commented 2 years ago

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

tegefaulkes commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

CMCDragonkai commented 2 years ago

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

CMCDragonkai commented 2 years ago

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 commented 2 years ago

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

  1. It is to address https://github.com/MatrixAI/Polykey/pull/445#issuecomment-1253162920 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 commented 2 years ago

@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.