From 988f343d49b6073bbb20f684418bc77480cf9dc1 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 14 Jun 2024 10:36:17 +0300 Subject: [PATCH 1/5] fix: ensure onConnect is always called --- lib/api/api-pipeline.js | 3 +-- lib/core/request.js | 8 ++++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/api/api-pipeline.js b/lib/api/api-pipeline.js index e64b3294965..322334e7848 100644 --- a/lib/api/api-pipeline.js +++ b/lib/api/api-pipeline.js @@ -145,7 +145,7 @@ class PipelineHandler extends AsyncResource { } onConnect (abort, context) { - const { ret, res } = this + const { res } = this if (this.reason) { abort(this.reason) @@ -153,7 +153,6 @@ class PipelineHandler extends AsyncResource { } assert(!res, 'pipeline cannot be retried') - assert(!ret.destroyed) this.abort = abort this.context = context diff --git a/lib/core/request.js b/lib/core/request.js index 78003038ba9..0eb19e2aea5 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -26,6 +26,7 @@ const { headerNameLowerCasedRecord } = require('./constants') const invalidPathRegex = /[^\u0021-\u00ff]/ const kHandler = Symbol('handler') +const noop = () => {} class Request { constructor (origin, { @@ -296,6 +297,13 @@ class Request { } this.aborted = true + if (!this.abort) { + // Make sure to always call onConnect even on errors to avoid + // a common user mistake of assuming that onConnect is always + // called before any other hook. + this[kHandler].onConnect(noop) + } + return this[kHandler].onError(error) } From 8e76fdfbf680a9751f717adc99a623aa3a160971 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Mon, 24 Jun 2024 10:59:45 +0200 Subject: [PATCH 2/5] fixup --- lib/handler/decorator-handler.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/handler/decorator-handler.js b/lib/handler/decorator-handler.js index 26f220c64cd..16dc035021e 100644 --- a/lib/handler/decorator-handler.js +++ b/lib/handler/decorator-handler.js @@ -1,7 +1,10 @@ 'use strict' +const noop = () => {} + module.exports = class DecoratorHandler { #handler + #onConnectCalled = false constructor (handler) { if (typeof handler !== 'object' || handler === null) { @@ -11,10 +14,15 @@ module.exports = class DecoratorHandler { } onConnect (...args) { + this.#onConnectCalled = true return this.#handler.onConnect?.(...args) } onError (...args) { + if (!this.#onConnectCalled) { + this.#handler.onConnect?.(noop) + this.#onConnectCalled = true + } return this.#handler.onError?.(...args) } From b14a4e9151ddfd60a6d7c9bcb47c1ad95bb24aba Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Mon, 24 Jun 2024 11:03:34 +0200 Subject: [PATCH 3/5] fixup --- lib/handler/decorator-handler.js | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/lib/handler/decorator-handler.js b/lib/handler/decorator-handler.js index 16dc035021e..9d2c4d0a34d 100644 --- a/lib/handler/decorator-handler.js +++ b/lib/handler/decorator-handler.js @@ -1,10 +1,13 @@ 'use strict' +const assert = require('node:assert') const noop = () => {} module.exports = class DecoratorHandler { #handler #onConnectCalled = false + #onCompleteCalled = false + #onErrorCalled = false constructor (handler) { if (typeof handler !== 'object' || handler === null) { @@ -20,33 +23,54 @@ module.exports = class DecoratorHandler { onError (...args) { if (!this.#onConnectCalled) { - this.#handler.onConnect?.(noop) this.#onConnectCalled = true + this.#handler.onConnect?.(noop) } + + this.#onErrorCalled = true return this.#handler.onError?.(...args) } onUpgrade (...args) { + assert(!this.#onCompleteCalled) + assert(!this.#onErrorCalled) + return this.#handler.onUpgrade?.(...args) } onResponseStarted (...args) { + assert(!this.#onCompleteCalled) + assert(!this.#onErrorCalled) + return this.#handler.onResponseStarted?.(...args) } onHeaders (...args) { + assert(!this.#onCompleteCalled) + assert(!this.#onErrorCalled) + return this.#handler.onHeaders?.(...args) } onData (...args) { + assert(!this.#onCompleteCalled) + assert(!this.#onErrorCalled) + return this.#handler.onData?.(...args) } onComplete (...args) { + assert(!this.#onCompleteCalled) + assert(!this.#onErrorCalled) + + this.#onCompleteCalled = true return this.#handler.onComplete?.(...args) } onBodySent (...args) { + assert(!this.#onCompleteCalled) + assert(!this.#onErrorCalled) + return this.#handler.onBodySent?.(...args) } } From b38e964378cfcbc68cf4dc3e3d8132fb896cd117 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Mon, 24 Jun 2024 19:14:18 +0200 Subject: [PATCH 4/5] Update lib/core/request.js --- lib/core/request.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/core/request.js b/lib/core/request.js index 0eb19e2aea5..2e7fdcd9e7c 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -297,12 +297,6 @@ class Request { } this.aborted = true - if (!this.abort) { - // Make sure to always call onConnect even on errors to avoid - // a common user mistake of assuming that onConnect is always - // called before any other hook. - this[kHandler].onConnect(noop) - } return this[kHandler].onError(error) } From e551e92522c8d6c7adde37b4872cdba927d9a7e1 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Mon, 24 Jun 2024 19:14:41 +0200 Subject: [PATCH 5/5] Apply suggestions from code review --- lib/core/request.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/core/request.js b/lib/core/request.js index 2e7fdcd9e7c..78003038ba9 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -26,7 +26,6 @@ const { headerNameLowerCasedRecord } = require('./constants') const invalidPathRegex = /[^\u0021-\u00ff]/ const kHandler = Symbol('handler') -const noop = () => {} class Request { constructor (origin, { @@ -297,7 +296,6 @@ class Request { } this.aborted = true - return this[kHandler].onError(error) }