Conversation
| server.listen(0, async () => { | ||
| const ac = new AbortController() | ||
| const ures = await request(`http://0.0.0.0:${server.address().port}`, { signal: ac.signal }) | ||
| const uresPromise = request(`http://0.0.0.0:${server.address().port}`, { signal: ac.signal }) |
There was a problem hiding this comment.
ℹ️ this test was wrong because request api terminates when the stream has been completely consumed, so the connection has been closed. If we postpone the await we can throw the abort before dispatch starts consuming the stream, so the request is actually aborted
| const ures = await request(`http://0.0.0.0:${server.address().port}`, { signal: ac.signal }) | ||
| ac.abort() | ||
| t.equal(await ures.body.text(), 'asd') |
There was a problem hiding this comment.
ℹ️ in this case the abort signal is ignored since the request is completed
There was a problem hiding this comment.
Since these are modeled after the fetch body mixins, I'm wondering if they should throw. In the fetch spec, if a request was aborted and you attempt to read the body, it'll throw an error. @ronag wdyt?
Uzlopak
left a comment
Uzlopak
left a comment
There was a problem hiding this comment.
makes kind of sense
RSLGTM
* upstream/main: fix: use explicit flag for when use has interacted with stream (nodejs#3361) refactor: simplify signal handling (nodejs#3362) fix: consider bytes read when dumping (nodejs#3360) websocket: don't use pooled buffer in mask pool (nodejs#3357) Revert "fix: post request signal (nodejs#3354)" (nodejs#3359) fix: post request signal (nodejs#3354) build(deps): bump node from `075a5cc` to `9af472b` in /build (nodejs#3355)
|
Hey, how can I add this fix to my nodejs? (I'm running latest and it's not there iiuc) |
6 participants
Fix post request abort signal
Fix issue #3353
Rationale
Request API doesn't remove the signal listener after complete the request
Changes
Remove abort listener after success
Status