fix(test): disable preventDefault() for beforeunload event

[nayeemrmn]

Fixes #18910.

[bartlomieju]

This seems more like a bandaid rather than fixing the underlying problem - I believe the root cause is the one described in #16928 that I wanted to fix in the coming days (we now have all the ingredients to properly fix it). I'm not in favor by altering the behavior of beforeunload events in deno bench/deno test. What do you say I try to fix it properly with #16928 first?

[nayeemrmn]

I don't think it's a band-aid. It's possible there are other issues that lead to the following block being needed:

deno/ext/node/polyfills/process.ts

Lines 736 to 742 in 10ae5ee

	globalThis.addEventListener("beforeunload", (e) => {
	process.emit("beforeExit", process.exitCode || 0);
	processTicksAndRejections();
	if (core.eventLoopHasMoreWork()) {
	e.preventDefault();
	}
	});

But while investigating it I noticed the inconsistency of whether or not the event loop is allowed to run to completion after a test run. This PR consolidates on the typical case where it doesn't, rather than only doing it if there's a beforeunload handler which prevents default. It happens to fix that issue.

[nayeemrmn]

The only alternative to this as I described in #18910 (comment) is to always call worker.run_event_loop() after the test loop. But I don't like that because it can lead to hanging with no info even though test results have been fully gathered.

[bartlomieju]

I don't think it's a band-aid. It's possible there are other issues that lead to the following block being needed:

deno/ext/node/polyfills/process.ts

Lines 736 to 742 in 10ae5ee

	globalThis.addEventListener("beforeunload", (e) => {
	process.emit("beforeExit", process.exitCode || 0);
	processTicksAndRejections();
	if (core.eventLoopHasMoreWork()) {
	e.preventDefault();
	}
	});

But while investigating it I noticed the inconsistency of whether or not the event loop is allowed to run to completion after a test run. This PR consolidates on the typical case where it doesn't, rather than only doing it if there's a beforeunload handler which prevents default. It happens to fix that issue.

Not really - there are no other issues that led to this block looking like it does, besides I had no other way to polyfill this event when this code lived in std/node. I'm fairly certain that I can rework it to not clash with "beforeunload".

At the end of the test run, it's true that there is more work in the event loop because the child process is still open. But for normal test runs we don't attempt to run the event loop unless a JS beforeunload handler calls e.preventDefault() (which the Node compat layer does I assume for good reason). This is kind of strange.

Again, there was no good reason, besides the fact that I wasn't able to polyfill it in any other way at the time of writing that code, which as the linked issue described is actually buggy.

We should either attempt to complete the event loop before dispatching beforeunload no matter what, or be ruthless and not allow the user to keep a worker alive once the tests from that module are finished i.e. ignore the defaultPrevented for beforeunload in test runs. I'm strongly leaning to the latter.

Maybe, still it feels a little strange that in deno run one can keep the event loop alive with beforeunload handler as long as needed/wanted but deno test and deno bench would prevent that.

@dsherret @lucacasonato thoughts on this?

[nayeemrmn]

there are no other issues that led to this block looking like it does

I was referring to #18910. Does the reproduction you're using for #16928 also use deno test? Otherwise I do think it's unrelated to this change and can continue to be tracked there.

This is related to a separate issue of whether or not to allow the event loop to complete in a test module after finishing tests. Currently deno test is not consistent with deno run. To make it clearer, the worker clean up code should either look like this:

  // Run event loop to completion, same as `deno run`.
  worker.run_event_loop(false).await?;
  loop {
    if !worker.dispatch_beforeunload_event(located_script_name!())? {
      break;
    }
    worker.run_event_loop(false).await?;
  }
  worker.dispatch_unload_event(located_script_name!())?;

Or like this:

  // Don't run event loop, run unload events but don't allow them to prevent default.
  !worker.dispatch_beforeunload_event(located_script_name!())?;
  worker.dispatch_unload_event(located_script_name!())?;

But currently it looks like this:

  // Don't run event loop typically, run unload events and run the event loop if they prevent default.
  loop {
    if !worker.dispatch_beforeunload_event(located_script_name!())? {
      break;
    }
    worker.run_event_loop(false).await?;
  }
  worker.dispatch_unload_event(located_script_name!())?;

I think the first one is disruptive because existing code which might rely on the worker and its open resources being dropped after tests are finished would start to hang with no info. The second is what would normally happen currently but with the edge case smoothed off. Thoughts?

[bartlomieju]

I was referring to #18910. Does the reproduction you're using for #16928 also use deno test? Otherwise I do think it's unrelated to this change and can continue to be tracked there.

No, the reproduction uses deno run.

I think the first one is disruptive because existing code which might rely on the worker and its open resources being dropped after tests are finished would start to hang with no info. The second is what would normally happen currently but with the edge case smoothed off. Thoughts?

Sorry for slow response. Wouldn't the first case immediately break the loop once beforeunload event is disptached and then dispatch unload event?. To be honest I don't fully understand why there's a difference how these loops look between deno run/deno bench/deno test.

I asked other team members to take a look at this PR. If there are no objections I'll land it as is. We can change the behavior if needed once I fix #16928.

[nayeemrmn]

Wouldn't the first case immediately break the loop once beforeunload event is disptached and then dispatch unload event?. To be honest I don't fully understand why there's a difference how these loops look between deno run/deno bench/deno test.

The relevant line is the invariant worker.run_event_loop() added at the top. Currently that's not there.

[bartlomieju]

Wouldn't the first case immediately break the loop once beforeunload event is disptached and then dispatch unload event?. To be honest I don't fully understand why there's a difference how these loops look between deno run/deno bench/deno test.

The relevant line is the invariant worker.run_event_loop() added at the top. Currently that's not there.

Exactly :D how does it even work currently if we're not polling the event loop? Am I missing something simple here? I guess it's because of worker.js_runtime.call_and_await? But in such case if there's a setup code at top level that uses async APIs it's not polled before the first test case runs, right?

[nayeemrmn]

I guess it's because of worker.js_runtime.call_and_await? But in such case if there's a setup code at top level that uses async APIs it's not polled before the first test case runs, right?

Yep, the same was true since runTests() in JS. There's no opportunity to run the event loop until awaiting the first test result and you can't do worker.run_event_loop() before running tests because it would never resolve if say you started a server for testing.

[bartlomieju]

I guess it's because of worker.js_runtime.call_and_await? But in such case if there's a setup code at top level that uses async APIs it's not polled before the first test case runs, right?

Yep, the same was true since runTests() in JS. There's no opportunity to run the event loop until awaiting the first test result and you can't do worker.run_event_loop() before running tests because it would never resolve if say you started a server for testing.

Okay, that makes sense. Sorry for back and forth on this.

[bartlomieju]

LGTM, once the comments are added

[bartlomieju]

LGTM