'Unfinished hook actions on exit' even printed when used via API

[timvahlbrock]

Expected Behavior / Situation

When rollup is executed using the API and the process is exited (maybe because a plugin failed), rollup should leave error handling and printing logs to the program that it has been called by.

Actual Behavior / Situation

Rollup prints 'Unfinished hook actions on exit' even when called using from the API, because the process exit handler, introduced in #4320 is always registered. If a plugin like @rollup/typescript is used, which is expected to throw errors from time to time, the original error message can be moved quite far up if there are more than a handful of unresolved hook actions. Printing of this message can only be prevented by not calling process.exit, which leaves the process running forever.

Apart from usability, I think it just shouldn't be in rollup's responsibility to decide what is printed on the console when it is used via its API.

Modification Proposal

Register the handler only if Rollup is used from the CLI. Optionally, allow API consumers to fetch unresolved hook actions, by either appending them to plugin errors or by providing an exposed API to fetch unfinished actions anytime.

[lukastaegert]

I see the problem, but it is not immediately clear how to design a good API here. We cannot expose them on the bundle/build because the problem is that they are relevant when the event loop is exhausted and thus the process just exits. Also, a plugin API seems over the top for something that is meant to debug plugin bugs. But maybe we can approach the problem from a different angle: Do not display them if the build finishes with a regular error. I assume you see them when your build aborts due to an actual error?

[timvahlbrock]

So the problem with "don't print them on a regular error" is, that the program, which is using rollup might actually want to exit with a non-zero exit code. In my case, this is because the build is started by another build tool (grunt), that needs a non-zero exit code to fail the build all together. In this scenario, exiting the program early with a non zero exit code is actually desired and not a bug.
I still see the problem more with rollup controlling the output when it's used via the API. According to #4320 this output was introduced to provide better debugging support for buggy plugins. I haven't quite got though under which circumstances these issues occurred and why it's rollup's responsibility to care for somewhat useable error messages. What would prevent the surrounding program from collecting the unfinished hooks over the rollup API (given it would be extended correspondingly)?

[lukastaegert]

What would prevent the surrounding program from collecting the unfinished hooks over the rollup API (given it would be extended correspondingly)?

The Rollup API works via Promises but if plugins exhaust the event loop (the problem is complicated, usually two plugins waiting for each other in a deadlock), then the Promise will never resolve and we cannot get any values to the caller. We found no real way to diagnose the situation except detecting a premature process.exit.
Without this, Rollup will just close without an error and without having created any output, which was a terrible situation (and happened quite a lot with some recent API additions).

Admittedly as this information is collected as static global state, we could also expose it via a separate named export. But this would mean everyone who is using Rollup via a script would need to look for this and learn about this error. However, we could also provide a way to silence these errors imperatively this way.

However, if the problem is only failing builds with a non-zero exit code, I think from the idea of the feature it would be completely fine to only display the error when we exit with a zero exit code as in the other case, there is already an error for the user to look at and they know not everything is fine.

[timvahlbrock]

I cannot quite comprehend how a promise deadlock would lead to a premature zero code exit, but not displaying the unfinished hooks on premature zero exits would do the trick for me.

[lukastaegert]

I cannot quite comprehend how a promise deadlock

In a nutshell, it is like

async function run() {
  await new Promise(() => { /* never resolves */ });
  console.log('this is never logged, instead we get a zero exit code');
}

run();

except that the non-resolving Promise is created in a much more elaborate and non-obvious way.

[timvahlbrock]

Ah. So I just read one or two things about the event loop, and that Node is "counting" asynchronous tasks, but as a never resolving promise might actually not do anything async, the event loop is empty. So node exits with 0, because there is no async operation left that could re-enter execution again.

I did found out, that there is a "beforeExit" event on node processes, which is executed when the event loot is emptied, but - in contrast to "exit" - can schedule async operations, reentering normal execution. Couldn't be used in someway like the following.

let resolve;
let reject;
const cancellationPromise = new Promise((res, rej) => {
    resolve = res;
    reject = rej;
});

async function rollup() {
    await Promise.all([
        new Promise((res) => { /* never resolves */ }),
        cancellationPromise,
    ]);
    console.log('this is never logged, instead we get a zero exit code');
}

process.on("beforeExit", (code) => {
    if(code == 0) {
        // if(unfinishedHookActions.length > 0)
        reject(new Error("Unexepected empty event loop!"))
    }
})

run();

If I got everything right, could a structure like this be used to reject the Promise of the rollup build when the node exits unexpectedly? If rollup is in CLI Mode, the rejection can be handled by the rollup CLI and printed like before. In case of API mode, either the calling program handles the error or bubbles into an unhandled promise rejection - which would be logged again by node. In both cases, the process exit hook would just be dumping to the console, but handing control to whoever called rollup.

Nevertheless, the key to solve this seems to be checking on the error code eitherway.

[timvahlbrock]

Just realized the example I provided would leave the build stuck even if the non-fulfulling promise would resolve, but I'm sure you get the idea.

[lukastaegert]

Actually, it is perfect! beforeExit is only called in the case we wanted to catch, it is not called at all for an explicit process.exit(code) no matter what code is. So good thing I explained our issue to you, thanks for the input! So if I do what you suggested except with a Promise.race, it will allow us to get rid of the console.error and use the existing error handling mechanism, which will also work much nicer in scripts.
I will see that I prepare a PR in the next days.

[lukastaegert]

Created #4434 to implement this