lib: implement AbortSignal.any()

[atlowChemi]

Fixes: #47811
Refs: whatwg/dom#1152

[benjamingr]

Don't we want to wait for browsers + the WhatWG DOM PR to land before implementing?

[benjamingr]

I think this can be simplified to:

function any(signals) {
  const ac = new AbortController();
  validateAbortSignalArray(signals, 'signals');
  for(const signal of signals) {
    if (signal.aborted) {
      ac.abort(signal);
      return ac.signal;
    }
  }
  for(const signal of signals) {
    signal.addEventListener("abort", (e) => {
      ac.signal.abort(e);
    }, { once: true, [kWeakListener]: ac.signal });
  }
  return ac.signal;
}

Since we already support weak listeners on event handlers inside core. You've effectively reimplemented parts of this in this PR and we can reuse the existing machinery.

[atlowChemi]

@benjamingr The implementation currently is spec compliant. do we want to implement it differently than the spec?

[benjamingr]

@atlowChemi the spec talks about behavior not about how we implement the spec behavior. We can be spec compliant even if we don't follow the spec machinery word for word as long as we have the same external API and the same behavior.

[atlowChemi]

@benjamingr Got it, thanks for the explanation 🙂
That said, I tried implementing based on your suggestion, but it causes the execution order to change (which is breaking the tests - as it doesn't comply with spec). I was unable to figure this out, any suggestions?

[UNEXPECTED_FAILURE][FAIL] Abort events for AbortSignal.any() signals fire in the right order (using AbortController)
assert_equals: expected "01234" but got "41230"
    at Test.<anonymous> (/Users/chemiatlow/Documents/node/test/fixtures/wpt/dom/abort/resources/abort-signal-any-tests.js:183:5)
    at Test.step (/Users/chemiatlow/Documents/node/test/fixtures/wpt/resources/testharness.js:2595:25)
    at test (/Users/chemiatlow/Documents/node/test/fixtures/wpt/resources/testharness.js:628:30)
    at abortSignalAnyTests (/Users/chemiatlow/Documents/node/test/fixtures/wpt/dom/abort/resources/abort-signal-any-tests.js:164:3)
    at /Users/chemiatlow/Documents/node/test/fixtures/wpt/dom/abort/abort-signal-any.any.js:4:1
Command: /Users/chemiatlow/Documents/node/out/Release/node  /Users/chemiatlow/Documents/node/test/wpt/test-abort.js 'abort-signal-any.any.js'

[benjamingr]

That's interesting and really surprising behavior for the signal "abort" to not be in "listener added order. We can work around it but I wonder if there is context regarding why they did this.

[benjamingr]

Worst case this is just event listener order and not hard to work around but I wanna understand why

[benjamingr]

See spec discussion in whatwg/dom#1152 (comment)

Just for completeness the way to work around it with my suggestion implementation without absorbing the additional complexity is to add the ability to tag event listeners as "run first" perhaps in an EventTarget subclass.

I generally think it's better to fix it in the spec but to be absolutely clear I feel strongly our behavior should be aligned with the spec as that was a big part of the motivation to implement AbortSignal and web standard more generally and diverging would defeat the purpose.

[atlowChemi]

@benjamingr I am not fully sure I follow the discussion in whatwg... As far as I understood, concerns were raised regarding using addEventListener related to user-land being able to call stopImmediatePropagation.
Were does this suggestion stand currently?
Should it be implemented?

[benjamingr]

We patiently wait (or engage in the discussion) until @shaseley and @annevk decide and then follow what they decide. I've raised my concerns there but I feel strongly we should follow whatever consensus that PR concludes with and the correct spec.

[benjamingr]

Good start!

[atlowChemi]

The tests seem to be failing due to #47635 (specifically the change from Map to Set which doesn't have a get function - https://github.com/nodejs/node/pull/47635/files#diff-044c4e3128d6bcf139f31a36cf054f2e59954367d4493e1c1b1a42d7b9d88067R481)
@MoLow @benjamingr @anonrig @panva What should I be doing about this?

[panva]

The tests seem to be failing due to #47635 (specifically the change from Map to Set which doesn't have a get function - #47635 (files)) @MoLow @benjamingr @anonrig @panva What should I be doing about this?

Good news is that #47826 deals with the runner issue. Bad news is that it's not why the tests are failing here. The parent process which executes the WPT files is exiting while the underlying wpt worker apparently still runs.

./node test/wpt/test-abort.js abort-signal-any.any.js
[PASS] AbortSignal.any() works with an empty array of signals
[PASS] AbortSignal.any() follows a single signal (using AbortController)
[PASS] AbortSignal.any() follows multiple signals (using AbortController)
[PASS] AbortSignal.any() returns an aborted signal if passed an aborted signal (using AbortController)
[PASS] AbortSignal.any() can be passed the same signal more than once (using AbortController)
[PASS] AbortSignal.any() uses the first instance of a duplicate signal (using AbortController)
[PASS] AbortSignal.any() signals are composable (using AbortController)
[PASS] AbortSignal.any() works with intermediate signals (using AbortController)
[PASS] Abort events for AbortSignal.any() signals fire in the right order (using AbortController)
[UNEXPECTED_FAILURE][INCOMPLETE] Unknown


{
  "abort-signal-any.any.js": {
    "fail": {
      "unexpected": [
        "Unknown"
      ]
    }
  }
}

Ran 1/1 tests, 0 skipped, 0 passed, 0 expected failures, 1 unexpected failures, 0 unexpected passes
/repo/node/test/common/wpt.js:748
        throw new Error(
        ^

Error: Found 1 unexpected failures. Consider updating test/wpt/status/dom/abort.json for these files:
abort-signal-any.any.js
    at process.<anonymous> (/repo/node/test/common/wpt.js:748:15)
    at process.emit (node:events:511:28)

Node.js v21.0.0-pre

[panva]

This particular test doesn't log a status

  async_test(t => {
    const controller = new controllerInterface();
    const timeoutSignal = AbortSignal.timeout(5);

    const combinedSignal = signalInterface.any([controller.signal, timeoutSignal]);

    combinedSignal.onabort = t.step_func_done(() => {
      assert_true(combinedSignal.aborted);
      assert_true(combinedSignal.reason instanceof DOMException,
          "combinedSignal.reason is a DOMException");
      assert_equals(combinedSignal.reason.name, "TimeoutError",
          "combinedSignal.reason is a TimeoutError");
    });
  }, `${desc} works with signals returned by AbortSignal.timeout() ${suffix}`);

[panva]

Using this branch, the test just never completes, onabort is not executed.

const assert = require('node:assert')

const controller = new AbortController();
const timeoutSignal = AbortSignal.timeout(5);

const combinedSignal = AbortSignal.any([controller.signal, timeoutSignal]);

combinedSignal.onabort = () => {
    // This never runs
    assert.equal(combinedSignal.aborted, true);
    assert.equal(combinedSignal.reason instanceof DOMException, true);
    assert.equal(combinedSignal.reason.name, "TimeoutError");
}

[panva]

Don't we want to wait for browsers + the WhatWG DOM PR to land before implementing?

I come with the same question, so far only chromium has an implementation but not in a stable release, so it's only available behind an experimental flag.

If this were marked as experimental in Node docs and had a runtime warning it would be fine. It can easily follow the spec as it evolves and the status can be changed when it lands in the DOM spec and others adopt it.

That would be the same that we do for https://wicg.github.io/webcrypto-secure-curves/ in WebCryptoAPI, it's marked as experimental in the docs and emits an experimental warning upon first use.

Tests of our own are also needed, running and passing WPTs are complimentary but in my opinion don't remove the need to thoroughly test the feature.

[jasnell]

I wouldn't worry about emitting a warning here.

[atlowChemi]

I am fine either way, but in previous comments I was asked to add it 🙂
#47821 (comment)
#47821 (comment)

[benjamingr]

I'm fine without a warning when the API lands at whatwg/dom but not before - so if you need someone who feels strongly about having an experimental warning until then - I'm that person :)

[benjamingr]

This is supposed to land Monday so as long as this lands after that I'm fine with it landing without a warning.

[atlowChemi]

Tests of our own are also needed, running and passing WPTs are complimentary but in my opinion don't remove the need to thoroughly test the feature.

@panva I added UT 🙂
I am currently struggling with the WPT test that is hanging (the test you mentioned here "the test just never completes, onabort is not executed."). I was not able to figure out how to connect a debugger to the test itself, but I see when running the same logic outside of the WPT scope it works fine. Do you have any suggestions? 🙏🏽

[panva]

Works fine as in the onabort block gets executed now? It didn't when I tried.

I have not tried attaching a debugger to WPTs.

[atlowChemi]

Works fine as in the onabort block gets executed now? It didn't when I tried.

Yes - but not when running via the WPT test, but as regular JS execution:

[panva]

Works fine as in the onabort block gets executed now? It didn't when I tried.

Yes - but not when running via the WPT test, but as regular JS execution:

const assert = require('node:assert');

const controller = new AbortController();
const timeoutSignal = AbortSignal.timeout(5);

const combinedSignal = AbortSignal.any([controller.signal, timeoutSignal]);

combinedSignal.onabort = () => {
  // This never runs
    console.log('foo')
    assert.equal(combinedSignal.aborted, true);
    assert.equal(combinedSignal.reason instanceof DOMException, true);
    assert.equal(combinedSignal.reason.name, "TimeoutError");
}

This still doesn't log for me, onabort is not run. Only when I add settimeout to the script. But the WPT doesn't, maybe it should, I don't know the spec enough to say.

cc @shaseley

[danielleadams]

Blocked by #47635