Don't use process.exit()

[dapplion]

💥 Proposal

In relation to the current SDK v7 refactor push, the frequent use of process.exit() in random parts of the code makes it really difficult to do integration tests

As a good practice, a Node.js app should not rely on process.exit() to stop a script. Whatever is holding the event loop alive should be cleaned, if possible.

For example dao apps is not able to finalize on it's own. The npm package node debugger wtfnode states that this 3 items are holding the event loop alive:

[WTF Node?] open handles:
- File descriptors: (note: stdio always exists)
  - fd 1 (tty) (stdio)
  - fd 2 (tty) (stdio)
- Sockets:
  - 127.0.0.1:42572 -> 127.0.0.1:8545

Proposal:

1. Never use process.exit(1) for errors. Throw an error and then handle it at the top yargs level with appropriate styling and debug / verbose settings
2. Understand what's keeping the event loop alive, and address the issue directly, allowing to have command handlers that don't end with process.exit(0)

[welcome]

Thanks for opening your first issue in aragonCLI! Someone will circle back soon ⚡

[sohkai]

The file handles we should definitely be closing... but I'm not super sure how we'll be able to close the web3 connection. I guess we can go to web3.currentProvider and try to invoke close() on it (not sure if web3@1.x implements close() on the providers but I think so).

[dapplion]

The file handles we should definitely be closing... but I'm not super sure how we'll be able to close the web3 connection. I guess we can go to web3.currentProvider and try to invoke close() on it (not sure if web3@1.x implements close() on the providers but I think so).

The first two file descriptors are integral parts of the node process so they are not a problem. Web3 1.x has indeed a way to close a websocket connection

web3.currentProvider.connection.close()

The problem now is to unify an control the multiple providers that are floating around

[dapplion]

After some research today I found this issues hanging the node process:

Web3 providers

Web3 WebSocket providers can be gracefully closed with

web3.currentProvider.connection.close()

We should uniform having only one initialized Web3 object in a CLI command lifecycle and call close() after the command completes (in the hook described below).

Known initializations of Web3 providers:

aragon-cli/packages/aragon-cli/src/lib/configEnvironment.js

Line 43 in d70b41b

provider: new Web3.providers.WebsocketProvider(

aragon-cli/packages/aragon-cli/src/lib/configEnvironment.js

Line 63 in d70b41b

: new Web3.providers.WebsocketProvider(

aragon-cli/packages/aragon-cli/src/lib/configEnvironment.js

Line 116 in d70b41b

? new Web3.providers.WebsocketProvider(wsProviderUrl)

aragon-cli/packages/aragon-cli/src/helpers/web3-fallback.js

Line 7 in d70b41b

web3 = new Web3(network.provider)

Really long timeouts

this line in the @aragon/apm is causing the AragonCLI to never stop. https://github.com/aragon/apm.js/blob/3d818929ce19410ea6a29e12964b19c08312b7a7/src/utils/timeout-promise.js#L6.
I can open a PR to patch just in this file by clearing the timeout started in case the first promise resolves. However I would advise to use a different pattern to timeout IPFS queries. I think the current ipfs-http-client has a configurable timeout time.

---

For reference, this yargs feature about to be released will be great to unify the final handlers
yargs/yargs#1473

.onFinishCommand(async (resultValue) => {
    await this.db.disconnect()
    process.exit()
}).argv;

[0xGabi]

@dapplion I started working on a branch to have a way to unify web3 initialization on the middleware level. In the same way that we currently handling the deprecated network global option in here. We could have a check for all command that doesn't initialize web3.
Then on configEnvironment.js we will have a new function configWeb3 where we:

1. Omit commands that don't use web3
2. Initialize web3 globally

I think we can have a similar approach for IPFS. What do you think? I'll try to have a draft PR soon 👍

[0xGabi]

The problem now is to unify an control the multiple providers that are floating around
I think now that we have all logic of an environment in one place on the toolkit it should be fairly easy to unify the multiple providers.

@dapplion if we initialize a toolkit instance with something like const toolkit = new Toolkit(environment) what you think would be the best way to close the connections once we no longer want to use that instance.

[dapplion]

If we use HTTP(s) there will be no need to close open connections. Since the toolkit should not subscribe to anything WebSockets are not mandatory.