polykey async-init fixes #281

tegefaulkes · 2021-10-29T03:51:26Z

For addressing point 1. #194 (comment)

Problem regarding the usage of src/config.ts and PolykeyAgent.ts. The PK_INGRESS_PORT has to be propagated from env variables to the ReverseProxy.createReverseProxy().

However we noticed during the integration of js-async-init, some things weren't done exactly the way it's supposed to be.

In PolykeyAgent, the structure at the end of createPolykeyAgent should be:

    const pk = new Polykey({ ... });
    await pk.start({ ... });
    return pk;

The reason to have an asynchronous createX static constructor is to call the start and ensure that constructed instances are started already. If this wasn't intended, then you only needed the StartStop decorator, not the CreateDestroyStartStop decorator.

The fact that PolykeyAgent wasn't done this way, may mean that other classes that are using the CreateDestroyStartStop pattern are also doing it incorrectly.

Now because the createX static methods are meant to call the asynchronous start, this means start parameters/options must be available in the createX static methods. The ReverseProxy has these parameters:

  public async start({
    ingressHost = '0.0.0.0' as Host,
    ingressPort = 0 as Port,
    serverHost,
    serverPort,
    tlsConfig,
  }: {
    ingressHost?: Host;
    ingressPort?: Port;
    serverHost: Host;
    serverPort: Port;
    tlsConfig: TLSConfig;
  }): Promise<void> {

These need to be replicated to the createReverseProxy.

@tegefaulkes

Note the difference between constructor parameters and start parameters. In the recent work on EFS and js-db, it's better to put parameters into the constructor if it is expected that they will stick around as class properties. Only if the parameters are temporary to the asynchronous start, then you should have them in the start method. This means there are some changes required to the PK domains that haven't yet been done.

In ReverseProxy, there are some cases where start does require parameters, because for example ingressPort can be set to 0, and then it has be resolved subsequently to the actual port, and that's why it is in the start method.

Tasks

The text was updated successfully, but these errors were encountered:

tegefaulkes · 2021-10-29T04:03:28Z

Review all uses of CreateDestroyStartStop decorator and ensure that their createX functions are calling start

create a list of all the domains using CreateDestroyStartStop.
determine if any domains are missing CreateDestroyStartStop pattern.
make sure all domains on the list are calling start() inside the createX function.

tegefaulkes · 2021-10-29T04:13:34Z

Referencing the diagram from https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205#note_688426664.

list of domains using CreateDestroyStartStop.

FwdProxy
RevProxy
NodeManager
PolykeyAgent
PolykeyClient
GRPCServer
Session - not converted.
GRPCClient - not converted

tegefaulkes · 2021-10-29T04:23:04Z

tegefaulkes · 2021-10-29T04:48:03Z

To finish the last two items on the above list I need to convert them to using CreateDestroyStartStop first.

Session
~~GRPCClient~~ This is an odd case. it was already converted. the interface was defined on this but the decoration with @CreateDestroyStartStop was done when it was extended for the client and the agent version of it.

CMCDragonkai · 2021-10-29T06:05:37Z

It's also PolykeyAgent. It wasn't propagating the start and stop and create and destroy.

tegefaulkes · 2021-10-29T06:35:45Z

I'm not sure we should have the networking modules start at creation. It means NodeMnanager can't be started before the services, GRPC and proxies are started. but also we're getting a dependency cycle where the clientService depends on PolykeyAgent, Polykeyagent depends on nodeManager and nodemanager depends on the service.
PolykeyAgent<-agentService<-grpcServer<-rev,fwdProxy<-NodeManager<-PolykeyAgent.

Since the networking modules are the most expensive to start up it makes sense that they and by extension PolykeyAgent should start independent of creation.

CMCDragonkai · 2021-10-31T03:49:24Z

@tegefaulkes did you make any WIP commits on the MR: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/213 or was there some unpushed work?

CMCDragonkai · 2021-10-31T04:40:30Z

The reason clientService depends on PolykeyAgent is due to all the agent-related commands right? Lke start and stop?

There may be a way to abstract this the there's circularity here.

CMCDragonkai · 2021-10-31T05:26:39Z

The CreateDestroy that is applied on KeyManager is also problematic.

The CreateDestroy should not have a create method. Only a createX static method should exist.

See the tests that use CreateDestroy, there's no mention of create method.

CMCDragonkai · 2021-10-31T05:27:48Z

See this: https://gist.github.com/CMCDragonkai/1dbf5069d9efc11585c27cc774271584. The gist explains the different uses of CreateDestroy.

joshuakarp · 2021-11-01T03:32:39Z

Moving the async-init fixes todo list from the top-level description of #194. Most of these points are already in the task list in this issue, but they're a little more fleshed out here:

Fix usage of the CreateDestroyStartStop decorator, as per Testnet Node Deployment (testnet.polykey.io) #194 (comment) (please read through this comment carefully). This will involve:
- - Review all uses of CreateDestroyStartStop decorator and ensure that their createX functions are calling start
  For example:
```
  public static async createPolykeyAgent( ... ) {
    ...
    const pk = new Polykey({ ... });
    await pk.start({ ... });
    return pk;
  }
```
That is, the assumption is that a class using the CreateDestroyStartStop pattern must return a started instance after the call to createX. If this wasn't intended, then you only needed the StartStop decorator, not the CreateDestroyStartStop decorator.
- - Move any parameter that is meant to be stored as a class property and doesn't require asynchronous operations on them to the constructor instead of start.
- - Propagate all remaining start parameters to the createX functions so that when calling create you can parameterise the asynchronous creation.
  For example, the ReverseProxy should be something like:
```
static async createReverseProxy({
  ingressHost = '0.0.0.0' as Host,
  ingressPort = 0 as Port,
  serverHost,
  serverPort,
  tlsConfig,
  connConnectTime = 20000,
  connTimeoutTime = 20000,
  logger,
}: {
  connConnectTime?: number;
  connTimeoutTime?: number;
  logger?: Logger;
}): Promise<ReverseProxy> {
  const logger_ = logger ?? new Logger('ReverseProxy');
  ...
  const revProxy = new ReverseProxy( ... );
  await revProxy.start( ... );
  return revProxy;
}

constructor({
  connConnectTime,
  connTimeoutTime,
  logger,
}: {
  connConnectTime: number;
  connTimeoutTime: number;
  logger: Logger;
}) {
  logger.info('Creating Reverse Proxy');
  this.logger = logger;
  this.connConnectTime = connConnectTime;
  this.connTimeoutTime = connTimeoutTime;
  this.logger.info('Created Reverse Proxy');
}

public async start({
  ingressHost = '0.0.0.0' as Host,
  ingressPort = 0 as Port,
  serverHost,
  serverPort,
  tlsConfig,
}: {
  ingressHost?: Host;
  ingressPort?: Port;
  serverHost: Host;
  serverPort: Port;
  tlsConfig: TLSConfig;
})
```
- - Ensure that createX is calling start on any encapsulated dependencies too. Ensure that their lifecycles are also managed in stop and destroy (i.e. call their respective stop/destroy functions here).
  - Note the distinction between an encapsulated dependency and an external reference
  - An encapsulated dependency is any optional parameter in createX/start (because if they aren't passed, we have to create them ourselves). The class is expected to manage the lifecycle of these dependencies.
  - An external reference is any required parameter. Therefore, no management of lifecycle is required (it's managed elsewhere - do not call start/stop/create/destroy on these references).
- - Incorporate these changes into testing
- - Create sequence diagram in accordance with the matroshka/babushka doll (i.e. clearly showing how and when everything is created and where they're injected in the lifecycle of Polykey)

CMCDragonkai · 2021-11-01T03:51:45Z

The current MR https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/213 shows how to use CreateDestroyStartStop for src/sessions/Session.ts.

Also in our meeting today we realised that we needed to make the networking related domains StartStop instead of CreateDestroyStartStop because of a loop occuring where the PolykeyAgent instance has to be accessed by the clientService.ts in order to stop/destroy the agent instance.

This can only be achieved by passing this from the start method of PolykeyAgent as the instance is not yet available when you call createX static methods of the dependencies of PolykeyAgent.

For classes using StartStop, they don't have a createX method, instead relying on the upstream start methods to propagate start calls.

This may apply to ForwardProxy, ReverseProxy, GRPCServer and more. This requires some prototyping to check what the case is.

CMCDragonkai · 2021-11-02T06:35:04Z

This will be fixed by https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/213

CMCDragonkai · 2021-11-03T02:16:59Z

From this comment: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/213#note_721681210

Regarding CreateDestroyStartStop. For both Session and SessionManager you need:

at least StartStop because you want to be able to stop without destroying underlying state

at least CreateDestroy becuase you want to be able to destroy the underlying state

THEREFORE you need both!

It would indicate that a number of places using CreateDestroy should be using CreateDestroyStartStop as long as the underlying state is something we may want to destroy. For Session and SessionManager this may be the case, but for things like notifications, acl, having a separate destroy from stop can be a convenience.

CMCDragonkai · 2021-11-08T11:56:07Z

I noticed this call:

    await utils.mkdirExists(fs, nodePath);

In the PolykeyAgent.createPolykeyAgent.

Now the problem is that usually I've been creating the directories in the start.

But with asynchronous creation, this is important if the directory being created is needed by the dependencies.

So the start is called at the very end, this is something that is relevant.

On the otherhand, if each dependency won't really need the state until they themselves are also calling start, then it should work to only keep the directory creation in the start.

This could mean that side effects don't actually occur until you call start. It's just that the createX methods call start at the very end which propagates all the way down.

I'm trying this out in https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/213

CMCDragonkai · 2021-11-08T11:56:40Z

Note that I removed the recursive option since they should only make the directory assuming the parent directory already exists. This is a good safe option.

CMCDragonkai · 2021-11-08T12:02:23Z

KeyManager - should be CreateDestroyStartStop because the underlying keys state can be destroyed separately from being stopped

CMCDragonkai · 2021-11-08T12:32:12Z

One of the things I had to do is figure out the order of parameters for propagating start to createX.

So now I've done:

required constructor parameters
required start parameters
optional constructor parameters
optional start parameters

Usually there should be no required start parameters...

CMCDragonkai · 2021-11-09T00:28:16Z

@tegefaulkes I've updated the task list regarding the async fixes.

CMCDragonkai · 2021-11-09T00:30:01Z

I noticed this call:
    await utils.mkdirExists(fs, nodePath);
In the PolykeyAgent.createPolykeyAgent.

Now the problem is that usually I've been creating the directories in the start.

But with asynchronous creation, this is important if the directory being created is needed by the dependencies.

So the start is called at the very end, this is something that is relevant.

On the otherhand, if each dependency won't really need the state until they themselves are also calling start, then it should work to only keep the directory creation in the start.

This could mean that side effects don't actually occur until you call start. It's just that the createX methods call start at the very end which propagates all the way down.

I'm trying this out in https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/213

I changed my mind on this, this should actually be in the createX. Because dependency starts may require it.

tegefaulkes · 2021-11-09T02:13:18Z

Fixed up the networking depencency cycle and cleaned up PolykeyAgent.ts

tegefaulkes · 2021-11-09T02:39:38Z

CMCDragonkai · 2021-11-10T08:42:51Z

Does the networking dependency cycle involve GRPCClient? It's currently still CDSS?

CMCDragonkai · 2021-11-10T08:53:09Z

Seems like it doesn't. The GRPCServer is now StartStop.

But it seems like GRPCClient should be CreateDestroy instead because there's no relevant destruction here.

CMCDragonkai · 2021-11-10T09:08:36Z

Hmm the solution to the async init problem on the abstract class is simply not to have the decorator on the abstract class. Instead decorator on GRPCClientClient and GRPCClientAgent and that should be enough. Plus it's indeed impossible to decorate abstract constructors. I tried it out.

CMCDragonkai · 2021-11-10T12:36:55Z

Actually to fix the problem of using async init for the GRPCClient we must follow this: MatrixAI/js-async-init#1 (comment)

Basically the GRPCClient should not be wrapped in a decorator, only the child classes should be, or only the parent class.

Furthermore, the GRPCClient or derived should only be using CreateDestroy not CreateDestroyStartStop, and not StartStop. Only GRPCServer needs the StartStop treatment.

tegefaulkes · 2021-11-11T00:50:35Z

Do we really need readiness testing for all of the domains? since its provided via the asyc-ini library we should assume it works? is there a need to replicate the test for each domain?

CMCDragonkai · 2021-11-11T02:02:26Z

Just a copy of this test that exists in Session.test.ts:

  test('session readiness', async () => {
    const session = await Session.createSession({
      sessionTokenPath: path.join(dataDir, 'token'),
      logger
    });
    await expect(session.destroy()).rejects.toThrow(sessionErrors.ErrorSessionRunning);
    // should be a noop
    await session.start();
    await session.stop();
    await session.destroy();
    await expect(session.start()).rejects.toThrow(sessionErrors.ErrorSessionDestroyed);
    await expect(session.readToken()).rejects.toThrow(sessionErrors.ErrorSessionNotRunning);
    await expect(session.writeToken('abc' as SessionToken)).rejects.toThrow(sessionErrors.ErrorSessionNotRunning);
  });

That's a CDSS test. CD should be even more simpler.

We can create a standard template and copy paste it for each one.

It's in order to ensure that the exceptions is being abided by. A quick sanity test for create destroy related work too.

CMCDragonkai · 2021-11-11T02:03:03Z

And no you don't need to test every single method. Just like a one or 2 methods. If there's a class with MANY methods, then just 1 or 2 methods. The simple ones. This test is not attempting to test every single method.

Although if there's a limited set of actual "ready" methods, then it can be useful there. See how the expectation is that the method throws an exception.

CMCDragonkai · 2021-11-11T04:56:16Z

@tegefaulkes I made a mistake on the logger default in static.

It should be this.name there, example:

  static async createSession({
    sessionTokenPath,
    fs = require('fs'),
    logger = new Logger(this.name),
    sessionToken,
    fresh = false,
  }:{

this.name is used in static, but this.constructor.name is used in instance methods.

tegefaulkes · 2021-11-11T07:44:56Z

Fixed the logger defaults.

CMCDragonkai · 2021-11-11T07:45:04Z

Awesome work @tegefaulkes!

Last thing to fix is the usage of these:


[nix-shell:~/Projects/js-polykey/src]$ ag 'new Logger\(this.constructor.name\)'
keys/KeyManager.ts
58:    logger = new Logger(this.constructor.name),

vaults/VaultManager.ts
76:    logger = new Logger(this.constructor.name),

vaults/VaultInternal.ts
41:    logger = new Logger(this.constructor.name),

gestalts/GestaltGraph.ts
53:    logger = new Logger(this.constructor.name),

acl/ACL.ts
26:    logger = new Logger(this.constructor.name),

notifications/NotificationsManager.ts
60:    logger = new Logger(this.constructor.name),

identities/providers/github/GitHubProvider.ts
38:    this.logger = logger ?? new Logger(this.constructor.name);

identities/IdentitiesManager.ts
35:    logger = new Logger(this.constructor.name),

nodes/NodeGraph.ts
50:    logger = new Logger(this.constructor.name),

nodes/NodeConnection.ts
62:    logger = new Logger(this.constructor.name),

nodes/NodeManager.ts
67:    logger = new Logger(this.constructor.name),

schema/Schema.ts
22:    logger = new Logger(this.constructor.name),

sigchain/Sigchain.ts
59:    logger = new Logger(this.constructor.name),

discovery/Discovery.ts
36:    logger = new Logger(this.constructor.name),

agent/GRPCClientAgent.ts
23:    logger = new Logger(this.constructor.name),

All of those should be this.name if they are in the static constructor.

CMCDragonkai · 2021-11-11T07:45:28Z

Ah I see you've done it.

CMCDragonkai · 2021-11-11T07:47:33Z

This can be closed once https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/213 is merged.

CMCDragonkai · 2021-11-15T07:15:25Z

Something was missed, unnecessary start calls.

For example:

    db = await DB.createDB({
      dbPath,
      logger,
      crypto: makeCrypto(keyManager),
    });
    await db.start();

The db.start is no longer required, since it is expected that when you create it asynchronously, it is started.

CMCDragonkai · 2021-11-15T07:26:29Z

Might need to make an exception regarding session where it is being used by GRPCClientClient. In this case we have a optional dependency that isn't then constructed and managed internally because it is truly optional. If it is not set, it's not used.

CMCDragonkai · 2021-11-15T07:55:27Z

Removed the unnecessary db.start now from tests. Some are still needed because there are tests, that test the stop and start of the db.

CMCDragonkai · 2021-11-17T07:57:46Z

There are some parts of code still checking whether the domains are running or not. This is no longer necessary due to async init creation. We always expect they to be running by the time we are using start. Especially for this:

    if (!this.db.running) {
      throw new dbErrors.ErrorDBNotRunning();
    }

Will be removing these as I see these.

CMCDragonkai · 2021-11-17T08:01:28Z


[nix-shell:~/Projects/js-polykey/src]$ ag 'this.db.running'
nodes/NodeManager.ts
134:    if (!this.db.running) {

sigchain/Sigchain.ts
98:    if (!this.db.running) {

Other places which has this.

CMCDragonkai · 2021-11-17T09:20:19Z

Note that protected methods do not need the @ready decorator, it is only applied on external methods.

CMCDragonkai · 2021-11-18T06:07:01Z

Should have a documented checklist on all the constraints on using async-init. This should be going to documentation.

CMCDragonkai · 2021-11-23T09:04:24Z

I've noticed all of the network/Connection* classes haven't been integrated with async-init pattern.

Since these are managed internally by the proxies, I'm leaving them out so we can merge for now. However a new issue will be created for this. #293

CMCDragonkai · 2021-11-24T14:17:17Z

This has been done with the merge of https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/213.

Left over issues are #293 and larger scale refactoring of nodes in #225.

Developer documentation should be written here regarding this usage.

joshuakarp assigned tegefaulkes Nov 1, 2021

joshuakarp mentioned this issue Nov 1, 2021

Testnet Node Deployment (testnet.polykey.io) #194

Closed

4 tasks

CMCDragonkai added the development Standard development label Nov 1, 2021

CMCDragonkai added the procedure Action that must be executed label Nov 9, 2021

This was referenced Nov 23, 2021

Integrate async-init pattern to network/Connection.ts #293

Closed

Refactoring Nodes Domain: NodeConnection & NodeConnectionManager and Fixing Bugs #225

Closed

CMCDragonkai closed this as completed Nov 24, 2021

CMCDragonkai added r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management r&d:polykey:core activity 3 Peer to Peer Federated Hierarchy labels Jul 24, 2022

polykey async-init fixes #281

polykey async-init fixes #281

Comments

tegefaulkes commented Oct 29, 2021 • edited Loading

Tasks

tegefaulkes commented Oct 29, 2021

Review all uses of CreateDestroyStartStop decorator and ensure that their createX functions are calling start

tegefaulkes commented Oct 29, 2021 • edited Loading

tegefaulkes commented Oct 29, 2021 • edited Loading

tegefaulkes commented Oct 29, 2021 • edited Loading

CMCDragonkai commented Oct 29, 2021

tegefaulkes commented Oct 29, 2021

CMCDragonkai commented Oct 31, 2021

CMCDragonkai commented Oct 31, 2021

CMCDragonkai commented Oct 31, 2021

CMCDragonkai commented Oct 31, 2021

joshuakarp commented Nov 1, 2021

CMCDragonkai commented Nov 1, 2021

CMCDragonkai commented Nov 2, 2021

CMCDragonkai commented Nov 3, 2021

CMCDragonkai commented Nov 8, 2021

CMCDragonkai commented Nov 8, 2021

CMCDragonkai commented Nov 8, 2021

CMCDragonkai commented Nov 8, 2021

CMCDragonkai commented Nov 9, 2021

CMCDragonkai commented Nov 9, 2021

tegefaulkes commented Nov 9, 2021

tegefaulkes commented Nov 9, 2021 • edited Loading

CMCDragonkai commented Nov 10, 2021

CMCDragonkai commented Nov 10, 2021

CMCDragonkai commented Nov 10, 2021

CMCDragonkai commented Nov 10, 2021

tegefaulkes commented Nov 11, 2021

CMCDragonkai commented Nov 11, 2021

CMCDragonkai commented Nov 11, 2021 • edited Loading

CMCDragonkai commented Nov 11, 2021

tegefaulkes commented Nov 11, 2021

CMCDragonkai commented Nov 11, 2021

CMCDragonkai commented Nov 11, 2021

CMCDragonkai commented Nov 11, 2021

CMCDragonkai commented Nov 15, 2021

CMCDragonkai commented Nov 15, 2021 • edited Loading

CMCDragonkai commented Nov 15, 2021

CMCDragonkai commented Nov 17, 2021

CMCDragonkai commented Nov 17, 2021

CMCDragonkai commented Nov 17, 2021

CMCDragonkai commented Nov 18, 2021

CMCDragonkai commented Nov 23, 2021 • edited Loading

CMCDragonkai commented Nov 24, 2021

tegefaulkes commented Oct 29, 2021 •

edited

Loading

tegefaulkes commented Oct 29, 2021 •

edited

Loading

tegefaulkes commented Oct 29, 2021 •

edited

Loading

tegefaulkes commented Oct 29, 2021 •

edited

Loading

tegefaulkes commented Nov 9, 2021 •

edited

Loading

CMCDragonkai commented Nov 11, 2021 •

edited

Loading

CMCDragonkai commented Nov 15, 2021 •

edited

Loading

CMCDragonkai commented Nov 23, 2021 •

edited

Loading