register a timeout in AbortSignal.any test

[panva]

Refs: nodejs/node#47821 (comment)

Because of the (i assume) weak references this test causes the Node.js worker that executes the suite to exit before this one runs to completion.

This change makes it so that a timeout is registered that should never be reached whilst still testing the combinedSignal behaviours when triggered by AbortSignal.timeout whilst keeping our test execution worker alive.

[annevk]

cc @shaseley

[shaseley]

I'm curious why this would be needed here but not in the AbortSignal.timeout() tests? How does the test execution stay alive for those tests?

[panva]

I'm curious why this would be needed here but not in the AbortSignal.timeout() tests? How does the test execution stay alive for those tests?

There just needs to be one that covers the duration of the other tests? 🤷‍♂️

I don't have the time to invest in this and only wish to unblock node.js landing the feature.

Edit:

this test, particularly its use of step_timeout, in AbortSignal.any.js does the same for that file, when I comment the test out, our runner behaves the same.

async_test(t => {
  const s = AbortSignal.abort();
  s.addEventListener("abort", t.unreached_func("abort event listener called"));
  s.onabort = t.unreached_func("abort event handler called");
  t.step_timeout(() => { t.done(); }, 2000);
}, "signal returned by AbortSignal.abort() should not fire abort event");

[panva]

Can we move this forward to unblock nodejs landing AbortSignal.any?

[annevk]

Hmm, it sounds like a bug that your engine doesn't run to completion here. I don't think we should add something that ends up hiding that.

[panva]

That's not an accurate assessment. The event loop has nothing to process and is finished, worker process exits. Other AbortSignal tests have a timeout in them too.

This addition does not alter the tested behaviour so please reconsider.

[annevk]

But there is something to process, no? The abort listener?

[panva]

I'm not that into the abortsignal spec but I find it strange that e.g. abortsignal.timeout would hang a process when passed to a fetch which has already finished.

[panva]

I'd hate to have to go back to the nodejs wpt worker and register global timeouts there just because the tests are written primarily for browsers that sit on a test url until completion or timeout.

[annevk]

Well that is not the scenario here. Here we have a signal with an active listener.

[aduh95]

Here we have a signal with an active listener.

I'm not sure I understand what active listener means in this context, are you saying that e.g. having new AbortController().signal.onabort = () => {}; somewhere in the code base should make the process hang forever? That would not be my expectation, and be quite surprising actually 🤔 Is there something in the spec that defines this or is it up to implementers?

[annevk]

That's a good question, it depends on how you define your event loop.

Looking at the one the HTML Standard defines for workers, I'm not sure how this PR ends up helping as the task queue would still be empty. Hmm. (Though for workers you do need to close it first, which doesn't happen fully automatically.)

(There's no difference in that respect between setTimeout() and AbortSignal.timeout().)