Promise deadlock when parsing commander actions

[ivomurrell]

Describe the bug

I'm currently writing some unit tests for a project I'm working on that includes forwarding CLI arguments to Webpack. We do this by calling the runCli function defined in packages/webpack-cli/lib/bootstrap.js. Typically this seems to behave as if you were running npx webpack on the command-line but I noticed when writing the tests that I was getting timeouts regardless of how long I set them with jest.setTimeout.

After some investigating I've found that the Promise returned by runCli never actually resolves, staying in a permanent pending state. This isn't noticeable when running the command normally as node will exit regardless of whether there are pending promises if nothing is on the event loop. However, in a standard unit test in jest or mocha there is a timer running on the event loop to timeout the test if it takes too long. This means that the Promise is never silently dropped and, paradoxically, guarantees the test will time out.

What is the current behavior?

Calling runCli returns with a Promise that never leaves the pending state. This means any code after an await runCli([...]) statement will not be executed.

To Reproduce

After initialising a barebones webpack project with

npm i webpack webpack-cli && mkdir src && echo "console.log(\"Hello world!\")" > src/index.js

just run

let promise = new require("webpack-cli/lib/bootstrap")([...process.argv, "build", "--mode=production"], require("module").prototype._compile)

in the Node REPL. You can then observe that promise stays as Promise { <pending> }, regardless of how long you wait. You may note the webpack compile stats are printed to the console and the bundle has been generated in dist but the Promise remains pending.

Expected behavior

runCli should resolve after the webpack actions have completed

Please paste the results of webpack-cli info here, and mention other relevant information

System:
    OS: macOS 11.4
    CPU: (8) x64 Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz
    Memory: 283.32 MB / 16.00 GB
  Binaries:
    Node: 12.22.1 - /usr/local/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 6.14.12 - /usr/local/bin/npm
  Browsers:
    Chrome: 91.0.4472.114
    Safari: 14.1.1

Additional context

The reason why the Promise never resolves is because webpack-cli recursively calls Commander.Command.parseAsync. It first calls parseAsync to execute an action for a webpack subcommand (like build or watch) and then calls it again on a child Command object to interpret the options passed to the subcommand. parseAsync is a simple function (defined here https://github.com/tj/commander.js/blob/v7.2.0/index.js#L1317) that waits on all the promises in its _actionResults array. After the first action is defined, it is pushed onto _actionResults and subsequently waited on when parseAsync is called. This action includes defining another action, however a new command is first created:

webpack-cli/packages/webpack-cli/lib/webpack-cli.js

Lines 103 to 107 in 9c6b341

	const command = this.program.command(commandOptions.name, {
	noHelp: commandOptions.noHelp,
	hidden: commandOptions.hidden,
	isDefault: commandOptions.isDefault,
	});

So in theory there shouldn't be any clash with the original Command. Unfortunately, Command.action actually pushes the action onto the _actionResults array of the root Command, in this case the original parent Command. This looks like behaviour that will change in Commander v8 (see tj/commander.js#1513) but for now means that both the parent and child Command calls to parseAsync end up waiting on the same Promise: deadlock!

[alexander-akait]

it is private API, what is use case?

[alexander-akait]

We do double parseAsync due lazy loading commands, unfortunately commander doesn't support this, so we do this rely on process.exit

[ivomurrell]

it is private API, what is use case?

We want to programmatically call webpack and pass command-line arguments so that it will parse options and load the config the same way it would as if you called from the CLI.

[ivomurrell]

We do double parseAsync due lazy loading commands, unfortunately commander doesn't support this, so we do this rely on process.exit

It doesn't seem like process.exit is ever called when a build is successful though

[alexander-akait]

It should https://github.com/webpack/webpack-cli/blob/master/packages/webpack-cli/lib/webpack-cli.js#L918

[ivomurrell]

It looks like the exitOverride callback is only called if the subcommand is an executable rather than an action handler:

• Callback called in _executeSubCommand: https://github.com/tj/commander.js/blob/v7.2.0/index.js#L1406 (callback only called elsewhere in the event of a help command, a version command, or an error)
• _executeSubCommand only called if command is an executable: https://github.com/tj/commander.js/blob/v7.2.0/index.js#L1441
• Difference between the two command types defined here: https://github.com/tj/commander.js/blob/v7.2.0/index.js#L598 and here: https://github.com/tj/commander.js/tree/v7.2.0#stand-alone-executable-subcommands

So in this case exitOverride is never called and the program usually just exits because of an empty event loop rather than process.exit being called.

[alexander-akait]

If you have idea how to fix, PR welcome, but it is private API and should not use directly

[alexander-akait]

Ideally we need this tj/commander.js#1276, but it as closed, maybe v8 solve the problem...

[ivomurrell]

Commander 8.0.0 just dropped and I'm happy to report the promise now resolves when following the reproduction steps 🎉

[anshumanv]

Awesome! Closing, feel free to open a new issue if you face any other problem.

[alexander-akait]

Let's keep open until we update

[anshumanv]

we need do it once we drop Node 10 👍

[alexander-akait]

Yep, I think we can start to work on it in the next branch - drop Node.js 10, drop webpack v4, update commander, simplify logic for options, use --entry-reset