Apply timedCancellable across the board to control how long side-effects are allowed to complete #450

Activity

1. tegefaulkes

   added

   developmentStandard developmentStandard development

   on Sep 21, 2022
2. tegefaulkes

   self-assigned this

   on Sep 21, 2022
3. CMCDragonkai

   changed the title [-]Fix errors due to short `pingNode` timeout[/-] [+]Apply `timedCancellable` across the board to control how long side-effects are allowed to complete[/+] on Sep 26, 2022
4. tegefaulkes commented on Sep 28, 2022

   

   tegefaulkes

   on Sep 28, 2022

   ContributorAuthor

   More actions

   This is going to be a pretty large change. Converting everything to TimedCancellable is pretty simple. there is just a lot of things to change and cover.

   1. Nodes domain handles a bunch of connections functionality for agent-agent communication. These have been converted to TimedCancellable already but needs to be reviewed again.
   2. Cancellablility needs to be integrated with the GRPC calls. I need to research how to do this, it should be possible to abort a connection. Generalizing this for all GRPC calls and error handling needs to be prototyped.
   3. Server side deadlines and abortion needs to be implemented as a complement to the client side aborting and deadlines.
   4. Proxy connections need to be updated to use the new style of cancellability.
5. tegefaulkes commented on Sep 28, 2022

   

   tegefaulkes

   on Sep 28, 2022

   ContributorAuthor

   More actions

   I don't think the default timeout for the timedCancellable decorator can be set at any time. It has to be defined at import time. This means we can't configure a default time when starting the NodeConnectionManager. We also don't want to have hard coded timeouts everywhere so we may have to define the defaults in the config file.
6. CMCDragonkai commented on Sep 29, 2022

   

   CMCDragonkai

   on Sep 29, 2022

   Member

   More actions

   There are several places where timedCancellable and variants of this should be used. Basically anywhere where there is non-deterministic side-effects.

   Pretty much all of them concern around network usage.

   1. Deadlines applied to GRPC client
   2. Deadlines applied to GRPC service handlers
   3. Propagating deadline cancellation to long-running computations and network calls
   4. Locking - this currently requires the Promise.race work arounds until https://github.com/MatrixAI/js-async-locks/issues/21

   For long running computations, we're talking about generators and related iterators. In these cases, the cancellation of generators hasn't been entirely worked out yet, and instead it is expected that you would stop consuming the generator (if you don't ask for the next item, it would continue computing). The only case where cancellation applies is when you do ask for the next item, but you must await for it, in which case you'd like to have the ability to do p.cancel(). However atm, we don't have this ability yet, so instead, you may need to pass an overall signal to the generator function and for it to signal cancellation the traditional way.

   Remember similar to async f functions, I believe async f* forces them to be regular promises and not our extended promises. But I don't remember if I confirmed this fact.

   As for the network, pretty much all network operations should be cancellable or timed cancellable if there's a timeout. This requires a big major refactor of the Proxy code to support this. There are multiple "default" timeouts currently configured during class construction. If we use the decorators, the defaults has to be set statically and cannot take in instance parameters.

   Alternatively you can also not use the decorators, and just directly use PromiseCancellable while taking in the signal. That would allow you to take the instance/constructor parameters as the defaults for the Proxy.

   So we have to decide whether to use static parameters and the decorators, or whether to swap to just using the PromiseCancellable and taking in the signal directly.

   It's important to understand that the decorators are for convenience, but they are not infinitely flexible, it trades off some flexibility for some better convenience.
7. CMCDragonkai commented on Sep 29, 2022

   

   CMCDragonkai

   on Sep 29, 2022

   Member

   More actions

   I don't think the default timeout for the timedCancellable decorator can be set at any time. It has to be defined at import time. This means we can't configure a default time when starting the NodeConnectionManager. We also don't want to have hard coded timeouts everywhere so we may have to define the defaults in the config file.

   Yes, because if decorators are static, then the parameters they take are static too. This is of course only necessary for "default" deadlines. Whatever is passed into the method/function will override it. If we do use static deadlines, then it does make sense to propagate these static parameters directly from the config object.

   I discussed that you can also use signals and promise cancellable directly in the method to avoid static parameterisation...
8. CMCDragonkai commented on Sep 29, 2022

   

   CMCDragonkai

   on Sep 29, 2022

   Member

   More actions

   I suggest we do a priority here.

   1. Start with the network domain, we should refactor this to support timers and cancellability (this may mean we can remove the existing timer utility)
   2. Refactor the tests to use fast check as well, and to change the timeouts and see how it behaves.
   3. Then work onto the GRPC client and server side deadlines

   Also additionally there's the identities domain, which also has network side effects from contacting identity providers, these should also start using timed cancellable too.
9. CMCDragonkai commented on Sep 29, 2022

   

   CMCDragonkai

   on Sep 29, 2022

   Member

   More actions

   I'm thinking there's sort of like 3 levels of defaults here:

   1. Static defaults on the decorators themselves inherited from src/config.ts
   2. Instance defaults coming from the construction of the class
   3. Method parameters that are passed from the caller

   I'm not sure if 2 is possible if we use the decorators, unless we change them up to look up parameters on the instance if the method parameter isn't passed in (that is is set to undefined). But we would need to pass in a string to indicate the name of the parameter to look up.
10. CMCDragonkai commented on Sep 29, 2022

    

    CMCDragonkai

    on Sep 29, 2022

    Last edited by CMCDragonkai

    Member

    More actions

    I think it is possible to do all 3.

    This is the function head:

    function timedCancellable(
      lazy: boolean = false,
      delay: number = Infinity,
      errorTimeoutConstructor: new () => Error = contextsErrors.ErrorContextsTimedTimeOut,
    )

    It's possible to do something like:

    function timedCancellable(
      lazy: boolean | string | symbol = false,
      delay: number | string | symbol = Infinity,
      errorTimeoutConstructor: new () => Error = contextsErrors.ErrorContextsTimedTimeOut,
    )

    And then do timedCancellable('lazyProp', delaySym). And later use the this[lazy] and this[delay] to acquire the parameters.

    // If it was `undefined`, it would be set to `false`
    if (typeof lazy !== 'boolean') {
      lazy = this[lazy];
    }
    
    // If it was `undefined`, it would be set to `Infinity`
    if (typeof delay !== 'number') {
      delay = this[delay];
    }

    However this pattern may not be scalable to other decorators which have actual string/symbol parameters...

    So it may be better to actually have a separate set of parameters to refer to the instance.

    function timedCancellable(
      lazy?: boolean,
      delay?: number,
      errorTimeoutConstructor: new () => Error = contextsErrors.ErrorContextsTimedTimeOut,
      defaults?: {
        lazy: 'lazyProp';
        delay: 'delayProp';
      }
    )

    But I find this syntax way too verbose...

    We could also use a more different type like arrow functions.

    function timedCancellable(
      lazy: boolean | (this: any) => boolean = false,
      delay: number | (this: any) => number = Infinity,
      errorTimeoutConstructor: new () => Error = contextsErrors.ErrorContextsTimedTimeOut,
    )

    It'd be unlikely for a decorator parameter to be an arrow function, but if they needed to differentiate, they could also check the number of parameters... etc.

    Notice the this is literally any here.

    Using it would be like:

    class X {
      lazyProp = true;
    
      @timedCancellable((this) => this.lazyProp)
      method() {
    
      }
    }
11. CMCDragonkai commented on Sep 29, 2022

    

    CMCDragonkai

    on Sep 29, 2022

    Member

    More actions

    The last style is likely the more flexible.
12. CMCDragonkai commented on Sep 29, 2022

    

    CMCDragonkai

    on Sep 29, 2022

    Last edited by CMCDragonkai

    Member

    More actions

    I've added related issues #243 and #242. They should be addressed together here.

    If there are going to be multiple PRs here, this should be made an epic and with subissues and subPRs.
13. CMCDragonkai

    added

    epicBig issue with multiple subissuesBig issue with multiple subissues

    on Sep 29, 2022
14. CMCDragonkai commented on Sep 29, 2022

    

    CMCDragonkai

    on Sep 29, 2022

    Member

    More actions

    Full work on this might take 1 sprint and a bit.
15. CMCDragonkai commented on Sep 29, 2022

    

    CMCDragonkai

    on Sep 29, 2022

    Member

    More actions

    The todos relating to cancellability should also be done in this issue. This issue is an epic so there are multiple subissues. @tegefaulkes you can add more issues into this epic as you see fit.
16. 14 remaining items

    Load more

17. CMCDragonkai commented on Apr 24, 2023

    

    CMCDragonkai

    on Apr 24, 2023

    Member

    More actions

    The deadline mechanism works in the JSON RPC system now. It's just a matter of doing agent migration. But also ensuring that the subissues here are completed.
18. CMCDragonkai

    added

    r&d:polykey:supporting activitySupporting core activitySupporting core activity

    on Jul 10, 2023
19. CMCDragonkai commented on Jul 10, 2023

    

    CMCDragonkai

    on Jul 10, 2023

    Member

    More actions

    Applying it to sigchain depends on agent migration.

    Applying it to identity management only requires bridging it to the underlying IO which can be just the fetch function provided by cross-fetch.
20. tegefaulkes

    removed their assignment

    on Dec 13, 2023
21. CMCDragonkai

    removed

    epicBig issue with multiple subissuesBig issue with multiple subissues

    on Aug 12, 2024
22. tegefaulkes commented on Oct 18, 2024

    

    tegefaulkes

    on Oct 18, 2024 – with Linear

    ContributorAuthor

    More actions

    This issue is very old and predates the network stack overhaul. I'm going to close it as not planned.
23. tegefaulkes

    closed this as not plannedon Oct 18, 2024
24. CMCDragonkai commented on Oct 18, 2024

    

    CMCDragonkai

    on Oct 18, 2024 – with Linear

    Member

    More actions

    This is still required. It's independent of networking. It's basically any kind of operation that is meant to be timed and cancellable. It's a general pattern. Of course new issues should be more specific.
25. CMCDragonkai

    added sub-issues on Oct 23, 2025

    • Reduce the timeout for establishing a Node Connection within the Discovery domain (by adding timer override to NodeConnectionManager) #353
    • Apply timedCancellable to Sigchain such as the deadline in nodes claim process #243
    • Update timedCancllable decorator to support dynamic timeouts #463
    • Update Proxy domain to use timedCancellable #464
    • Update Nodes domain to used timedCancellable #465
    • Update GRPC client calls and service handlers to support cancellations and deadlines #466
    • Update Discovery domain to use timedCancellable #477