Specification
Currently we have 2 connection abstractions in PK:
network - Connection and ConnectionForward and ConnectionReverse - uses StartStopgrpc & nodes - GRPCClient, GRPCClientAgent, GRPCClientClient and NodeConnection - uses CreateDestroy
As you can see the network is using the StartStop pattern and the grpc & nodes is using CreateDestroy.
The CreateDestroy is fundamentally more limiting than StartStop, as it enforces a sort of logical order, where asynchronous side-effects must occur first before you are given the actual object instance. Thus tying the object-lifetime to asynchronous setup and shutdown.
Originally I wanted to apply CreateDestroy to network domain, but was impacted by some cyclic dependencies that existed in network domain. However now there are cyclic dependencies in the grpc and nodes domain, and it appears that CreateDestroy is too limiting, and instead we need to use StartStop.
Now what are these cyclic dependencies? They all boil down to needing to do some asynchronous side-effects while having access to the instance itself.
For example in ConnectionForward, the start call makes use of this.utpSocketandthis.tlsSocket, which are all properties of the instance. Some complex refactoring could move this all into async creation, and then passed in as properties of the ConnectionForward` instance, but this is unnecessary complexity when all these properties have to be part of the instance.
In the case of NodeConnection, we have a problem where it causes us to construct the instance first new NodeConnection() and then setup the GRPCClient and then inject the client object into the NodeConnection instance.
const nodeConnection = new NodeConnection<T>({
host: targetHost,
port: targetPort,
hostname: targetHostname,
fwdProxy: fwdProxy,
destroyCallback,
logger,
});
// ... setting up client
// TODO: This is due to chicken or egg problem
// see if we can move to CreateDestroyStartStop to resolve this
nodeConnection.client = client;The reason for this is because the creation of the GRPCClient requires the destroyCallback to be set, and that requires a reference to the nodeConnection instance.
This round-about way of doing things makes things a bit strange/convoluted for no reason.
If the creation logic of GRPCClient and NodeConnection were instead changed to StartStop pattern, then we can proceed to make this logic much more cleaner.
It does mean you may have NodeConnection instances that are not started. But that's ok, since our network proxies handle this by removing themselves from the connection proxies when they are stopped. The nodes connection make use of a generic destroyCallback to do this same behaviour. The main difference is that network connections hardcode knowledge about the connection state from the proxies, and require it during construction. See the connections parameter in ConnectionForward and ConnectionReverse. With GRPCClient, this is more difficult since nodes and grpc are separate domains, so we don't want to hardcode knowledge about nodes connection management into the grpc classes.
Additional context
Tasks
- Change
GRPCClient and GRPCClientClient, GRPCClientAgent and GRPCClientTest to StartStop
- Change
NodeConnection to StartStop
- Adapt the
NodeConnectionManager, and base it on how ForwardProxy and ReverseProxy handles it
- Update all tests
Specification
Currently we have 2 connection abstractions in PK:
network-ConnectionandConnectionForwardandConnectionReverse- usesStartStopgrpc&nodes-GRPCClient,GRPCClientAgent,GRPCClientClientandNodeConnection- usesCreateDestroyAs you can see the
networkis using theStartStoppattern and thegrpc&nodesis usingCreateDestroy.The
CreateDestroyis fundamentally more limiting thanStartStop, as it enforces a sort of logical order, where asynchronous side-effects must occur first before you are given the actual object instance. Thus tying the object-lifetime to asynchronous setup and shutdown.Originally I wanted to apply
CreateDestroytonetworkdomain, but was impacted by some cyclic dependencies that existed innetworkdomain. However now there are cyclic dependencies in thegrpcandnodesdomain, and it appears thatCreateDestroyis too limiting, and instead we need to useStartStop.Now what are these cyclic dependencies? They all boil down to needing to do some asynchronous side-effects while having access to the instance itself.
For example in
ConnectionForward, thestart call makes use ofthis.utpSocketandthis.tlsSocket, which are all properties of the instance. Some complex refactoring could move this all into async creation, and then passed in as properties of theConnectionForward` instance, but this is unnecessary complexity when all these properties have to be part of the instance.In the case of
NodeConnection, we have a problem where it causes us to construct the instance firstnew NodeConnection()and then setup theGRPCClientand then inject the client object into theNodeConnectioninstance.The reason for this is because the creation of the
GRPCClientrequires thedestroyCallbackto be set, and that requires a reference to thenodeConnectioninstance.This round-about way of doing things makes things a bit strange/convoluted for no reason.
If the creation logic of
GRPCClientandNodeConnectionwere instead changed toStartStoppattern, then we can proceed to make this logic much more cleaner.It does mean you may have
NodeConnectioninstances that are not started. But that's ok, since ournetworkproxies handle this by removing themselves from the connection proxies when they are stopped. Thenodesconnection make use of a genericdestroyCallbackto do this same behaviour. The main difference is thatnetworkconnections hardcode knowledge about the connection state from the proxies, and require it during construction. See theconnectionsparameter inConnectionForwardandConnectionReverse. WithGRPCClient, this is more difficult sincenodesandgrpcare separate domains, so we don't want to hardcode knowledge aboutnodesconnection management into thegrpcclasses.Additional context
NodeManagertoNodeConnectionManager#310 - PR that extracted node connection management intoNodeConnectionManagerIdjs-id#13 (comment) - Some discussion about polymorphicthis, this may not be relevant if we useStartStopTasks
GRPCClientandGRPCClientClient,GRPCClientAgentandGRPCClientTesttoStartStopNodeConnectiontoStartStopNodeConnectionManager, and base it on howForwardProxyandReverseProxyhandles it