Repository navigation
node:http: refresh the socket inactivity timer in the native write path #43753
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
b1c82c3
70db310
a6c21b4
f59ae7e
34d193e
2fb29ab
e45cb54
f276904
670efcd
a86db78
4a922e7
941a2fe
b41aca6
f31f4ac
ed93ca2
a79f34f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -921,7 +921,12 @@ struct HttpResponse : public AsyncSocket<SSL> { | |
| bool chunked = !(httpResponseData->state & (HttpResponseData<SSL>::HTTP_WROTE_CONTENT_LENGTH_HEADER | HttpResponseData<SSL>::HTTP_ANCIENT_REQUEST | HttpResponseData<SSL>::HTTP_CLOSE_DELIMITED)); | ||
|
|
||
| if (!data.empty()) { | ||
| /* With nothing buffered the tail is sent directly, which AsyncSocket::write() does not record. */ | ||
| size_t bufferedBefore = Super::getBufferedAmount(); | ||
| Super::write(data.data(), data.length()); | ||
| if (Super::getBufferedAmount() < bufferedBefore + data.length()) { | ||
| httpResponseData->backlogMoved = true; | ||
|
Comment on lines
+925
to
+928
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
| } | ||
| if (chunked) { | ||
| Super::write("\r\n", 2); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| // What the compiler reads in place of src/js/node/_http_server.ts, which `bun run` evaluates. The two do the same. | ||
| // Written against that file with the SHA-256 ff5fd0cf3ffd5e9a3fb246c83dbbefe854e635a64472500199657149d7819c82. | ||
| // Written against that file with the SHA-256 afe1fa727e188edb03f07d531760d32176b6cf4ddc3b2febb8751ce16f71a6d5. | ||
| // When that file changes, bring the change here and write the new hash: test/internal/second-texts-of-modules.test.ts. | ||
|
|
||
| // Hardcoded module "node:_http_server" | ||
|
|
@@ -2741,13 +2741,18 @@ function getNodeHTTPServerSocket(): NodeHTTPServerSocketConstructor { | |
|
|
||
| _onTimeout() { | ||
| const handle = this[kHandle]; | ||
| const response = handle?.response; | ||
| // If there is a response, and it has pending data, | ||
| // we suppress the timeout because a write is in progress. | ||
| if (response && response.writableLength! > 0) { | ||
| // Re-arm so the timeout still fires once the write actually stalls. | ||
| this._unrefTimer(); | ||
| return; | ||
| if (handle) { | ||
| // The connection has closed, and 'close' comes next. | ||
| if (handle.closed) return; | ||
| // Node looks at the write queue here: https://github.com/nodejs/node/blob/v26.3.0/lib/net.js#L596-L611 | ||
| // Not under the keep-alive timeout: nothing writes there, and the native connection does not have that timer. | ||
| if ( | ||
| !this[kKeepAliveTimeoutSet] && | ||
| handle.holdTimeoutBackOnce(this.writableLength > 0 || (handle.response?.bufferedAmount ?? 0) > 0) | ||
| ) { | ||
| this._unrefTimer(); | ||
| return; | ||
| } | ||
| } | ||
| this.emit("timeout"); | ||
| } | ||
|
|
@@ -2968,6 +2973,7 @@ function getNodeHTTPServerSocket(): NodeHTTPServerSocketConstructor { | |
| clearTimeout(existingTimer); | ||
| this[kSocketTimeoutTimer] = undefined; | ||
| this.$armedTimeout = 0; | ||
| this[kHandle]?.setInactivityTimer(undefined); | ||
| } | ||
| if (callback !== undefined) { | ||
| validateFunction(callback, "callback"); | ||
|
|
@@ -2985,6 +2991,10 @@ function getNodeHTTPServerSocket(): NodeHTTPServerSocketConstructor { | |
| this[kSocketTimeoutTimer] = timer; | ||
| this.$armedTimeout = msecs; | ||
| } | ||
| // Server writes refresh it there (Node: https://github.com/nodejs/node/blob/v26.3.0/lib/net.js#L1019). Never the keep-alive timer: nothing writes under it. | ||
| if (!this[kKeepAliveTimeoutSet]) { | ||
| this[kHandle]?.setInactivityTimer(this[kSocketTimeoutTimer]); | ||
| } | ||
| if (callback !== undefined) { | ||
| validateFunction(callback, "callback"); | ||
| this.once("timeout", callback); | ||
|
|
@@ -3881,13 +3891,18 @@ function onResponseFinishHandleSocket( | |
| // idle period started; onSocketTimeoutTimerExpired grants the remaining | ||
| // budget if the timer fires early, so the socket still closes after | ||
| // exactly `total` ms of idle. | ||
| if (socket.$armedTimeout === total || isArmedWith(socket, total)) { | ||
| // The native connection has the timer of a timeout that was armed for this request. Nothing writes under the keep-alive one. | ||
| const armed = socket.timeout !== 0; | ||
| if (armed) socket[kHandle]?.setInactivityTimer(undefined); | ||
| // Before setTimeout(), which hands any other timer to the native connection. | ||
| socket[kKeepAliveTimeoutSet] = true; | ||
| // The timer of an armed timeout can have fired, with a 'timeout' listener that kept the socket: setTimeout() starts it again. | ||
| if (!armed && (socket.$armedTimeout === total || isArmedWith(socket, total))) { | ||
| socket.timeout = total; | ||
| } else { | ||
| socket.setTimeout(total); | ||
| } | ||
| socket[kKeepAliveIdleStart] = DateNow(); | ||
| socket[kKeepAliveTimeoutSet] = true; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -4970,7 +4985,8 @@ const finishExchange = function emitResponseFinished(exchange: Exchange): void { | |
| return finishExchangeInSteps(exchange, flags); | ||
| } | ||
| const idleTimeout = keepAliveTimeout + keepAliveTimeoutBuffer; | ||
| if (socket.$armedTimeout !== idleTimeout && socket.setTimeout !== serverSocketSetTimeout) { | ||
| // finishConnection() calls setTimeout() in these two cases. | ||
| if ((socket.$armedTimeout !== idleTimeout || socket.timeout !== 0) && socket.setTimeout !== serverSocketSetTimeout) { | ||
|
Comment on lines
+4988
to
+4989
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| ("unlikely"); | ||
| return finishExchangeInSteps(exchange, flags); | ||
| } | ||
|
|
@@ -5067,13 +5083,19 @@ const finishConnection = function onResponseFinishHandleSocket( | |
| if (parser != null && parser.incoming === req && req.readableEnded) parser.incoming = null; | ||
| } | ||
| const idleTimeout = exchange.idleTimeout; | ||
| if (socket.$armedTimeout === idleTimeout || isArmedWith(socket, idleTimeout)) socket.timeout = idleTimeout; | ||
| else { | ||
| // The native connection has the timer of a timeout that was armed for this request. Nothing writes under the keep-alive one. | ||
| const armed = socket.timeout !== 0; | ||
| if (armed) socket[kHandle]?.setInactivityTimer(undefined); | ||
| // Before setTimeout(), which hands any other timer to the native connection. | ||
| socket[kKeepAliveTimeoutSet] = true; | ||
| // The timer of an armed timeout can have fired, with a 'timeout' listener that kept the socket: setTimeout() starts it again. | ||
| if (!armed && (socket.$armedTimeout === idleTimeout || isArmedWith(socket, idleTimeout))) { | ||
| socket.timeout = idleTimeout; | ||
| } else { | ||
| ("unlikely"); | ||
| socket.setTimeout(idleTimeout); | ||
| } | ||
| socket[kKeepAliveIdleStart] = DateNow(); | ||
| socket[kKeepAliveTimeoutSet] = true; | ||
| if (exchange.detachedFrom === null) { | ||
| if (socket._httpMessage === res) { | ||
| socket[kCloseCallback] = undefined; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.