lib: fix beforeExit not working with -e

[bnoordhuis]

Commit 93a44d5 ("src: fix deferred events not working with -e") defers
evaluation of the script to the next tick.

A side effect of that change is that 'beforeExit' listeners run before
the actual script. 'beforeExit' is emitted when the event loop is
empty but process.nextTick() does not ref the event loop.

Fix that by using setImmediate(). Because it is implemented in terms
of a uv_check_t handle, it interacts with the event loop properly.

Fixes: #8534

R=@Fishrock123

[Fishrock123]

patch LGTM, but isn't there still a larger issue that this is necessary?

That is, normal modules don't seem to need this, so why does -e?

[Fishrock123]

Actually, the test is timing out on Windows: https://ci.nodejs.org/job/node-test-binary-windows/4085/RUN_SUBSET=1,VS_VERSION=vs2015,label=win2012r2/tapTestReport/test.tap-28/

[imyller]

CI: https://ci.nodejs.org/job/node-test-pull-request/4359/

[bnoordhuis]

@nodejs/platform-windows Any ideas on why the test times out on Windows? Example: https://ci.nodejs.org/job/node-test-binary-windows/4154/RUN_SUBSET=1,VS_VERSION=vcbt2015,label=win10/console

[joaocgreis]

Investigated, but this was not easy as it first seemed to be. The test hangs in a live loop printing beforeExit to stdout. Apparently it's an unrelated issue, opened #9067 with my findings.

[joaocgreis]

As shown in #9067 , using console.log in beforeExit creates a loop. @bnoordhuis perhaps change the test to something like

  const script = `
      var beforeExitCalled = false;
      process.on("beforeExit", () => {beforeExitCalled = true});
      process.on("exit", () => {if(beforeExitCalled) console.log("ok")});
  `;

(This seems to work on Windows: fails without and passes with the change on this PR.)

[bnoordhuis]

Aw, and I even commented on that issue. I must have been having a brain boo boo that day.

Thanks for the pointer. Rebased and updated, PTAL. I switched the beforeExit listener from .on() to .once().

CI: https://ci.nodejs.org/job/node-test-pull-request/4546/

[bnoordhuis]

Green! (Well, yellow.) @cjihrig @Fishrock123 Still LGTY?

normal modules don't seem to need this, so why does -e?

The module system does it too, see Module.runMain(). The equivalent for -e would be this:

diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js
index 8b8d066..e778cb4 100644
--- a/lib/internal/bootstrap_node.js
+++ b/lib/internal/bootstrap_node.js
@@ -338,13 +338,9 @@
                    'return require("vm").runInThisContext(' +
                    `${JSON.stringify(body)}, { filename: ` +
                    `${JSON.stringify(name)}, displayErrors: true });\n`;
-    // Defer evaluation for a tick.  This is a workaround for deferred
-    // events not firing when evaluating scripts from the command line,
-    // see https://github.com/nodejs/node/issues/1600.
-    process.nextTick(function() {
-      const result = module._compile(script, `${name}-wrapper`);
-      if (process._print_eval) console.log(result);
-    });
+    const result = module._compile(script, `${name}-wrapper`);
+    if (process._print_eval) console.log(result);
+    process._tickCallback();
   }

   // Load preload modules

setImmediate() and process._tickCallback() are functionally identical here as far as I can tell

[cjihrig]

Not sure if the template string in the test should be indented 2 or 4 spaces, but yes, still LGTM.

[bnoordhuis]

Not sure if the template string in the test should be indented 2 or 4 spaces

I see both styles in the code base and the linter isn't complaining so I decided to leave it as-is.