feat: add abort signal to body.dump()

[debadree25]

Attempt to implement body.dump({ abort });

fixes: #1978

[KhafraDev]

there are failures in v12 and v14 due to the optional chaining

[debadree25]

Removed @KhafraDev

[codecov-commenter]

Codecov Report

Patch coverage: 94.44% and project coverage change: +0.04 🎉

Comparison is base (bc8e2b8) 90.42% compared to head (5018f29) 90.46%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1993      +/-   ##
==========================================
+ Coverage   90.42%   90.46%   +0.04%     
==========================================
  Files          71       71              
  Lines        6119     6157      +38     
==========================================
+ Hits         5533     5570      +37     
- Misses        586      587       +1     

Impacted Files	Coverage Δ
lib/api/readable.js	91.40% <92.30%> (+0.02%)	⬆️
lib/core/util.js	97.54% <100.00%> (-0.50%)	⬇️
lib/websocket/connection.js	88.31% <0.00%> (-0.94%)	⬇️
lib/fileapi/encoding.js	2.22% <0.00%> (-0.11%)	⬇️
lib/timers.js	100.00% <0.00%> (ø)
lib/client.js	97.55% <0.00%> (+<0.01%)	⬆️
lib/fetch/request.js	88.03% <0.00%> (+0.05%)	⬆️
lib/websocket/websocket.js	92.89% <0.00%> (+0.07%)	⬆️
lib/fetch/index.js	84.93% <0.00%> (+0.18%)	⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[debadree25]

Still fails 😕, looks like unrelated tests?

[debadree25]

hmm probably not unrelated, investigating

[ronag]

https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/throwIfAborted

[ronag]

Not perfect. As it won't cancel the iteration promise. But good enough.

Maybe a .destroy() call on abort can help?

[debadree25]

Something like

signal.addEventListener('abort', () => {
        this.destroy()
})

[debadree25]

Also not sure how to address the failures,
I tried using AbortController from require('abort-controller') but when using that the test simply hangs

[debadree25]

Updated to destroy body on abort

[ronag]

from node:

const validateAbortSignal = hideStackFrames((signal, name) => {
  if (signal !== undefined &&
      (signal === null ||
       typeof signal !== 'object' ||
       !('aborted' in signal))) {
    throw new ERR_INVALID_ARG_TYPE(name, 'AbortSignal', signal);
  }
});

[debadree25]

Quite a few CI fails again, looking into these in a while

[debadree25]

Does this not somehow inject AbortController for tests on older versions, i see this pattern being used in tests for eg: test/fetch/client-fetch.js

[KhafraDev]

no, it's just telling prettier that AbortController is a global

[debadree25]

Any ideas then @KhafraDev what could we do about this test?

[debadree25]

It seems using throwIfAborted would fail for the 16.8 version because the method wasnt released on 16.8 line

[debadree25]

Is there a way to rerun CI, the failures in 18 and 19 look unrelated

[ronag]

Can you make a fallback throwIfAborted? It needs to throw an error with name AbortError.

[debadree25]

Yes doing so

[debadree25]

Seems everything passing finally 🎉

[KhafraDev]

I don't think adding a public method here is a good solution, it'd be better to add it as a utility function

function throwIfAborted (signal) {
  if (signal.throwIfAborted) {
    signal.throwIfAborted()
  } else {
    // ...
  }
}

[debadree25]

Updating

[debadree25]

Updated, does it look ok?

[ronag]

This event listener needs to be cleaned up or we have a memory leak.

[debadree25]

Made this { once: true }

[ronag]

Could we move this into the util file? throwIfAborted(signal)

[debadree25]

Done!

[ronag]

this will still leak if we completed without abort

[ronag]

add a finally clause to remove it

[debadree25]

Ah sorry, completely distracted need more chai sigh, anyway have updated