fix: signal handling (#3053)

nodejs · Apr 7, 2024 · b6aa794 · b6aa794
1 parent c886b34
commit b6aa794
Show file tree

Hide file tree

Showing 7 changed files with 42 additions and 29 deletions.
diff --git a/lib/api/abort-signal.js b/lib/api/abort-signal.js
@@ -8,11 +8,14 @@ function abort (self) {
   if (self.abort) {
     self.abort(self[kSignal]?.reason)
   } else {
-    self.onError(self[kSignal]?.reason ?? new RequestAbortedError())
+    self.reason = self[kSignal]?.reason ?? new RequestAbortedError()
   }
+  removeSignal(self)
 }
 
 function addSignal (self, signal) {
+  self.reason = null
+
   self[kSignal] = null
   self[kListener] = null
 

diff --git a/lib/api/api-connect.js b/lib/api/api-connect.js
@@ -1,7 +1,8 @@
 'use strict'
 
+const assert = require('node:assert')
 const { AsyncResource } = require('node:async_hooks')
-const { InvalidArgumentError, RequestAbortedError, SocketError } = require('../core/errors')
+const { InvalidArgumentError, SocketError } = require('../core/errors')
 const util = require('../core/util')
 const { addSignal, removeSignal } = require('./abort-signal')
 
@@ -32,10 +33,13 @@ class ConnectHandler extends AsyncResource {
   }
 
   onConnect (abort, context) {
-    if (!this.callback) {
-      throw new RequestAbortedError()
+    if (this.reason) {
+      abort(this.reason)
+      return
     }
 
+    assert(this.callback)
+
     this.abort = abort
     this.context = context
   }

diff --git a/lib/api/api-pipeline.js b/lib/api/api-pipeline.js
@@ -147,12 +147,14 @@ class PipelineHandler extends AsyncResource {
   onConnect (abort, context) {
     const { ret, res } = this
 
-    assert(!res, 'pipeline cannot be retried')
-
-    if (ret.destroyed) {
-      throw new RequestAbortedError()
+    if (this.reason) {
+      abort(this.reason)
+      return
     }
 
+    assert(!res, 'pipeline cannot be retried')
+    assert(!ret.destroyed)
+
     this.abort = abort
     this.context = context
   }

diff --git a/lib/api/api-request.js b/lib/api/api-request.js
@@ -1,10 +1,8 @@
 'use strict'
 
+const assert = require('node:assert')
 const { Readable } = require('./readable')
-const {
-  InvalidArgumentError,
-  RequestAbortedError
-} = require('../core/errors')
+const { InvalidArgumentError } = require('../core/errors')
 const util = require('../core/util')
 const { getResolveErrorBodyCallback } = require('./util')
 const { AsyncResource } = require('node:async_hooks')
@@ -69,10 +67,13 @@ class RequestHandler extends AsyncResource {
   }
 
   onConnect (abort, context) {
-    if (!this.callback) {
-      throw new RequestAbortedError()
+    if (this.reason) {
+      abort(this.reason)
+      return
     }
 
+    assert(this.callback)
+
     this.abort = abort
     this.context = context
   }

diff --git a/lib/api/api-stream.js b/lib/api/api-stream.js
@@ -1,11 +1,8 @@
 'use strict'
 
+const assert = require('node:assert')
 const { finished, PassThrough } = require('node:stream')
-const {
-  InvalidArgumentError,
-  InvalidReturnValueError,
-  RequestAbortedError
-} = require('../core/errors')
+const { InvalidArgumentError, InvalidReturnValueError } = require('../core/errors')
 const util = require('../core/util')
 const { getResolveErrorBodyCallback } = require('./util')
 const { AsyncResource } = require('node:async_hooks')
@@ -70,10 +67,13 @@ class StreamHandler extends AsyncResource {
   }
 
   onConnect (abort, context) {
-    if (!this.callback) {
-      throw new RequestAbortedError()
+    if (this.reason) {
+      abort(this.reason)
+      return
     }
 
+    assert(this.callback)
+
     this.abort = abort
     this.context = context
   }

diff --git a/lib/api/api-upgrade.js b/lib/api/api-upgrade.js
@@ -1,6 +1,6 @@
 'use strict'
 
-const { InvalidArgumentError, RequestAbortedError, SocketError } = require('../core/errors')
+const { InvalidArgumentError, SocketError } = require('../core/errors')
 const { AsyncResource } = require('node:async_hooks')
 const util = require('../core/util')
 const { addSignal, removeSignal } = require('./abort-signal')
@@ -34,10 +34,13 @@ class UpgradeHandler extends AsyncResource {
   }
 
   onConnect (abort, context) {
-    if (!this.callback) {
-      throw new RequestAbortedError()
+    if (this.reason) {
+      abort(this.reason)
+      return
     }
 
+    assert(this.callback)
+
     this.abort = abort
     this.context = null
   }

diff --git a/lib/mock/mock-utils.js b/lib/mock/mock-utils.js
@@ -8,7 +8,7 @@ const {
   kOrigin,
   kGetNetConnect
 } = require('./mock-symbols')
-const { buildURL, nop } = require('../core/util')
+const { buildURL } = require('../core/util')
 const { STATUS_CODES } = require('node:http')
 const {
   types: {
@@ -285,10 +285,10 @@ function mockDispatch (opts, handler) {
     const responseHeaders = generateKeyValues(headers)
     const responseTrailers = generateKeyValues(trailers)
 
-    handler.abort = nop
-    handler.onHeaders(statusCode, responseHeaders, resume, getStatusText(statusCode))
-    handler.onData(Buffer.from(responseData))
-    handler.onComplete(responseTrailers)
+    handler.onConnect?.(err => handler.onError(err), null)
+    handler.onHeaders?.(statusCode, responseHeaders, resume, getStatusText(statusCode))
+    handler.onData?.(Buffer.from(responseData))
+    handler.onComplete?.(responseTrailers)
     deleteMockDispatch(mockDispatches, key)
   }