feat: add abort signal to body.dump() #1993

debadree25 · 2023-03-06T18:49:43Z

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

fixes: #1978

KhafraDev · 2023-03-06T22:47:04Z

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

debadree25 · 2023-03-07T05:06:38Z

Removed @KhafraDev

codecov-commenter · 2023-03-07T05:11:21Z

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 · 2023-03-07T05:21:54Z

Still fails 😕, looks like unrelated tests?

debadree25 · 2023-03-07T05:43:49Z

hmm probably not unrelated, investigating

ronag · 2023-03-07T08:30:35Z

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 · 2023-03-07T09:17:30Z

Something like

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

debadree25 · 2023-03-07T09:19:40Z

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 · 2023-03-07T09:25:05Z

Updated to destroy body on abort

lib/api/readable.js

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);
  }
});

Co-authored-by: Robert Nagy <[email protected]>

lib/api/readable.js

Co-authored-by: Robert Nagy <[email protected]>

debadree25 · 2023-03-07T10:19:49Z

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

lib/api/readable.js

Co-authored-by: Robert Nagy <[email protected]>

debadree25 · 2023-03-07T11:53:03Z

test/client-request.js

@@ -1,3 +1,5 @@
+/* globals AbortController */


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

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

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

debadree25 · 2023-03-07T20:12:13Z

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

debadree25 · 2023-03-07T20:13:12Z

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

ronag · 2023-03-08T12:33:44Z

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

debadree25 · 2023-03-08T12:57:46Z

Yes doing so

debadree25 · 2023-03-08T19:17:12Z

Seems everything passing finally 🎉

KhafraDev · 2023-03-08T19:30:50Z

lib/api/readable.js

+      }
+      if (!signal.throwIfAborted) {
+        // add in a throwIfAborted method to the signal
+        signal.throwIfAborted = function () {


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 { // ... } }

Updated, does it look ok?

ronag · 2023-03-09T07:48:44Z

lib/api/readable.js

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


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

Made this { once: true }

ronag · 2023-03-09T07:49:00Z

lib/api/readable.js

@@ -146,15 +146,39 @@ module.exports = class BodyReadable extends Readable {

  async dump (opts) {
    let limit = opts && Number.isFinite(opts.limit) ? opts.limit : 262144
+    const signal = opts && opts.signal
+    function throwIfAborted () {


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

ronag · 2023-03-09T08:27:47Z

lib/api/readable.js

      signal.addEventListener('abort', () => {
        this.destroy()
-      })
+      }, { once: true })


this will still leak if we completed without abort

add a finally clause to remove it

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

lib/core/util.js

Co-authored-by: Robert Nagy <[email protected]>

feat: add abort signal to body.dump()

f22d69f

fixup

7365982

fixup

6b3aadc

fixup! use throwIfAborted

d68c87c

ronag approved these changes Mar 7, 2023

View reviewed changes

fixup! destroy on abort

d9f67a1

ronag reviewed Mar 7, 2023

View reviewed changes

lib/api/readable.js Outdated Show resolved Hide resolved

ronag reviewed Mar 7, 2023

View reviewed changes

lib/api/readable.js Show resolved Hide resolved

ronag reviewed Mar 7, 2023

View reviewed changes

lib/api/readable.js Outdated Show resolved Hide resolved

ronag reviewed Mar 7, 2023

View reviewed changes

lib/api/readable.js Show resolved Hide resolved

ronag reviewed Mar 7, 2023

View reviewed changes

debadree25 and others added 5 commits March 7, 2023 15:14

Update lib/api/readable.js

e6f6ef0

Co-authored-by: Robert Nagy <[email protected]>

fixup! add validation for abort signal

1e5904e

Update lib/api/readable.js

9fdda11

Co-authored-by: Robert Nagy <[email protected]>

Update lib/api/readable.js

7761f8b

Co-authored-by: Robert Nagy <[email protected]>

Update lib/api/readable.js

d9157e1

Co-authored-by: Robert Nagy <[email protected]>

ronag reviewed Mar 7, 2023

View reviewed changes

lib/api/readable.js Outdated Show resolved Hide resolved

ronag reviewed Mar 7, 2023

View reviewed changes

lib/api/readable.js Outdated Show resolved Hide resolved

debadree25 and others added 3 commits March 7, 2023 15:22

fixup

7ad89f6

Update lib/api/readable.js

4f43953

Co-authored-by: Robert Nagy <[email protected]>

lint

f950347

ronag reviewed Mar 7, 2023

View reviewed changes

lib/api/readable.js Outdated Show resolved Hide resolved

lib/api/readable.js Outdated Show resolved Hide resolved

ronag reviewed Mar 7, 2023

View reviewed changes

lib/api/readable.js Outdated Show resolved Hide resolved

debadree25 and others added 3 commits March 7, 2023 16:12

Update lib/api/readable.js

823f688

Co-authored-by: Robert Nagy <[email protected]>

Update lib/api/readable.js

9dd400a

Co-authored-by: Robert Nagy <[email protected]>

Update lib/api/readable.js

b0c58cd

Co-authored-by: Robert Nagy <[email protected]>

debadree25 commented Mar 7, 2023

View reviewed changes

debadree25 added 2 commits March 9, 2023 00:30

fixup! use fallback throwOnError

e57948e

fixup! possible test fix

072db99

KhafraDev requested changes Mar 8, 2023

View reviewed changes

fixup! use as utility

7ca1bf3

KhafraDev approved these changes Mar 8, 2023

View reviewed changes

ronag requested changes Mar 9, 2023

View reviewed changes

debadree25 added 2 commits March 9, 2023 13:36

fixup! use listener once

7a72cca

fixup! move to util

ae2d712

ronag reviewed Mar 9, 2023

View reviewed changes

lib/core/util.js Outdated Show resolved Hide resolved

debadree25 and others added 2 commits March 9, 2023 14:18

Update lib/core/util.js

e5de075

Co-authored-by: Robert Nagy <[email protected]>

fixup! also remove on finally

5018f29

debadree25 requested a review from ronag March 10, 2023 10:10

ronag merged commit 65eea9b into nodejs:main Mar 10, 2023

metcoder95 mentioned this pull request Apr 12, 2023

feat: Add H2 support metcoder95/undici#33

Closed

7 tasks

metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Jul 21, 2023

feat: add abort signal to body.dump() (nodejs#1993)

98264c6

crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024

feat: add abort signal to body.dump() (nodejs#1993)

cfd0adb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add abort signal to body.dump() #1993

feat: add abort signal to body.dump() #1993

debadree25 commented Mar 6, 2023

KhafraDev commented Mar 6, 2023

debadree25 commented Mar 7, 2023

codecov-commenter commented Mar 7, 2023 •

edited

Loading

debadree25 commented Mar 7, 2023

debadree25 commented Mar 7, 2023

ronag commented Mar 7, 2023

ronag left a comment

debadree25 commented Mar 7, 2023

debadree25 commented Mar 7, 2023

debadree25 commented Mar 7, 2023

ronag left a comment

debadree25 commented Mar 7, 2023 •

edited

Loading

debadree25 Mar 7, 2023

KhafraDev Mar 7, 2023

debadree25 Mar 8, 2023

debadree25 commented Mar 7, 2023

debadree25 commented Mar 7, 2023

ronag commented Mar 8, 2023

debadree25 commented Mar 8, 2023

debadree25 commented Mar 8, 2023

KhafraDev Mar 8, 2023 •

edited

Loading

debadree25 Mar 8, 2023

debadree25 Mar 8, 2023

ronag Mar 9, 2023

debadree25 Mar 9, 2023

ronag Mar 9, 2023

debadree25 Mar 9, 2023

ronag Mar 9, 2023 •

edited

Loading

ronag Mar 9, 2023

debadree25 Mar 9, 2023

feat: add abort signal to body.dump() #1993

feat: add abort signal to body.dump() #1993

Conversation

debadree25 commented Mar 6, 2023

KhafraDev commented Mar 6, 2023

debadree25 commented Mar 7, 2023

codecov-commenter commented Mar 7, 2023 • edited Loading

Codecov Report

debadree25 commented Mar 7, 2023

debadree25 commented Mar 7, 2023

ronag commented Mar 7, 2023

ronag left a comment

Choose a reason for hiding this comment

debadree25 commented Mar 7, 2023

debadree25 commented Mar 7, 2023

debadree25 commented Mar 7, 2023

ronag left a comment

Choose a reason for hiding this comment

debadree25 commented Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debadree25 commented Mar 7, 2023

debadree25 commented Mar 7, 2023

ronag commented Mar 8, 2023

debadree25 commented Mar 8, 2023

debadree25 commented Mar 8, 2023

KhafraDev Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 7, 2023 •

edited

Loading

debadree25 commented Mar 7, 2023 •

edited

Loading

KhafraDev Mar 8, 2023 •

edited

Loading

ronag Mar 9, 2023 •

edited

Loading