Exit process after running a command

[spalladino]

Fixes #1469

[spalladino]

I've already cherry-picked this one to release/2.8 to publish a new RC with this fix.

[ylv-io]

Nice. I was wonder that was the reason it is missing. I guess there was no reason. Also why it is just a process.exit(0) but not if (!options.dontExitProcess && process.env.NODE_ENV !== 'test') process.exit(0);?

[ylv-io]

LGTM!

[spalladino]

Also why it is just a process.exit(0) but not if (!options.dontExitProcess && process.env.NODE_ENV !== 'test') process.exit(0)?

dontExitProcess was used when one action called into another (like create calling into push), which is no longer the case in the new implementation. And the NODE_ENV check is not needed because this code has no tests at all (see #1457).

By the way, I'd like for @frangio to check this PR before merging.

[frangio]

What is the underlying reason why it's necessary to call process.exit? I don't think it's good practice to exit the process like this. There could be pending work in the event queue.

I think someone mentioned once that the problem is in the HD wallet provider. Have we looked into fixing that? Because this is not a problem in CLI, but rather in the user setup, caused by a dependency that they installed and configured (granted, recommended by our documentaiton).

[spalladino]

I don't think it's good practice to exit the process like this. There could be pending work in the event queue.

The bulk of the work is currently awaited in the CLI. But it's true that there could be something else we are not considering which gets aborted.

Because this is not a problem in CLI, but rather in the user setup, caused by a dependency that they installed and configured (granted, recommended by our documentaiton).

While I agree, it is perceived as a problem with the CLI by our users, especially since other CLIs do include the process.exit to handle this provider. I would love to actually build a custom solution (see #1336) which is more lightweight, but it's on the backlog now. In the meantime, we need to provide a built-in solution.

Have we looked into fixing that?

In the hdwallet case, I think the issue is with provider-engine. The hdwallet-provider is built using provider-engine, which requires to be stop()'d once it finishes processing. We could add an action at the end of the CLI that inspects if the current provider implements a stop method, and if so, calls it.

However, we had run into issues with other libraries in the past that also have dangling connections (I think a version of truffle-contracts or web3 was one, which came up again during test-env). All in all, just calling process.exit was the cleanest way to wrap things up and ensure that the process would not hang, regardless of issues with our (or our users') dependencies.

To sum up, I'm fine with not adding this process.exit and fixing this particular case (by adding the stop() call), since we have reduced the number of dependencies we need to deal with within the CLI.

[frangio]

Approving regardless of my thoughts above because we have to support this at least for now.

[frangio]

Thanks for explaining @spalladino. I've created an issue to review this situation. #1483