fetch() AbortSignal.timeout() does not work as expected

[mcollina]

Bug Description

It seems passing { signal: AbortSignal.timeout(50) } to fetch() is not taken into consideration.

Reproducible By

Run node test/fetch/abort.js. The AbortSignal.timeout() test should pass immediately but it waits for 30s. This is the output:

 Subtest: Allow the usage of custom implementation of AbortController
    ok 1 - should be equal
    1..1
ok 1 - Allow the usage of custom implementation of AbortController # time=6.82ms

# Subtest: allows aborting with custom errors
    # Subtest: Using AbortSignal.timeout with cause
        1..2
        ok 1 - should be equal
        ok 2 - should be equal
    ok 1 - Using AbortSignal.timeout with cause # time=30057.744ms

    # Subtest: Error defaults to an AbortError DOMException
        ok 1 - expect rejected Promise
        1..1
    ok 2 - Error defaults to an AbortError DOMException # time=7.816ms

    1..2
ok 2 - allows aborting with custom errors # time=30072.88ms

1..2
# time=30085.421ms

Expected Behavior

The fetch request should be completed in the expected timeout.

Environment

Node 18 and 19, on both Windows and Mac OS X.

[mcollina]

cc @KhafraDev

[KhafraDev]

Did a bisect, this is caused by f035bbc. cc @ronag

[debadree25]

It seems, that the gc cleans up ac lot before it is used

from some console logs

TAP version 13
# Subtest: Allow the usage of custom implementation of AbortController
    ok 1 - should be equal
    1..1
ok 1 - Allow the usage of custom implementation of AbortController # time=13.181ms

# Subtest: allows aborting with custom errors
    # Subtest: Using AbortSignal.timeout with cause
        1..2
cleaned
aborting undefined
aborted

I was wondering if using the aborted() utility introduced in nodejs/node#46494 would be helpful here? I tried something like this

diff --git a/lib/fetch/request.js b/lib/fetch/request.js
index 9764871..5a0e17d 100644
--- a/lib/fetch/request.js
+++ b/lib/fetch/request.js
@@ -30,6 +30,7 @@ const { URLSerializer } = require('./dataURL')
 const { kHeadersList } = require('../core/symbols')
 const assert = require('assert')
 const { setMaxListeners, getEventListeners, defaultMaxListeners } = require('events')
+const { aborted } = require('util');
 
 let TransformStream = globalThis.TransformStream
 
@@ -354,17 +355,12 @@ class Request {
       if (signal.aborted) {
         ac.abort(signal.reason)
       } else {
-        const acRef = new WeakRef(ac)
-        const abort = function () {
-          acRef.deref()?.abort(this.reason)
-        }
 
         if (getEventListeners(signal, 'abort').length >= defaultMaxListeners) {
           setMaxListeners(100, signal)
         }
 
-        signal.addEventListener('abort', abort, { once: true })
-        requestFinalizer.register(this, { signal, abort })
+        aborted(signal, this).then(() => ac.abort(signal.reason))
       }
     }

Intuition suggests that when this is gc'd all the references to the promise and the references to ac in the then method would also be gc'd but I am unsure as there are no good ways to test the original issue #1823

[stalkerg]

Sorry, but this change breaks my service.

[stalkerg]

@debadree25

I am unsure as there are no good ways to test the original issue #1823

in the original issue, in the description, vary the small portion of code to reproduce it. Basically, it happened constantly if the argument of the fetch is Request