-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
http: merge chunks during chunked encoding #47630
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
09ea79c
db9b222
8e9a0b5
cf2cc8a
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 |
---|---|---|
|
@@ -866,6 +866,7 @@ function strictContentLength(msg) { | |
); | ||
} | ||
|
||
let chunkBuffer = Buffer.alloc(0); | ||
function write_(msg, chunk, encoding, callback, fromEnd) { | ||
if (typeof callback !== 'function') | ||
callback = nop; | ||
|
@@ -930,11 +931,8 @@ function write_(msg, chunk, encoding, callback, fromEnd) { | |
|
||
let ret; | ||
if (msg.chunkedEncoding && chunk.length !== 0) { | ||
len ??= typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength; | ||
msg._send(NumberPrototypeToString(len, 16), 'latin1', null); | ||
msg._send(crlf_buf, null, null); | ||
msg._send(chunk, encoding, null, len); | ||
ret = msg._send(crlf_buf, null, callback); | ||
chunkBuffer = Buffer.concat([chunkBuffer, Buffer.from(chunk)]); | ||
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. Concatenating buffers is very slow. They require a memory allocation + memory copy. Just keep an |
||
ret = true; | ||
} else { | ||
ret = msg._send(chunk, encoding, callback, len); | ||
} | ||
|
@@ -1046,6 +1044,14 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) { | |
throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(this[kBytesWritten], this._contentLength); | ||
} | ||
|
||
if (!this._headerSent && Buffer.byteLength(chunkBuffer)) { | ||
this._send(NumberPrototypeToString(Buffer.byteLength(chunkBuffer), 16), 'latin1', null); | ||
this._send(crlf_buf, null, null); | ||
this._send(chunkBuffer, null, null); | ||
this._send(crlf_buf, null, callback); | ||
chunkBuffer = Buffer.alloc(0) | ||
} | ||
Comment on lines
+1047
to
+1053
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. This should be done in nextTick (i.e., connectionCorkNT) otherwise, there is no point to all of this... the number of buffered chunks would always be 1. |
||
|
||
const finish = onFinish.bind(undefined, this); | ||
|
||
if (this._hasBody && this.chunkedEncoding) { | ||
|
@@ -1062,7 +1068,6 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) { | |
this.socket.uncork(); | ||
} | ||
this[kCorked] = 0; | ||
|
||
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. unnecessary change |
||
this.finished = true; | ||
|
||
// There is the first message on the outgoing queue, and we've sent | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
/* | ||
In the first line of the test file you should | ||
enable strict mode, unless you test something | ||
that needs it disabled | ||
*/ | ||
'use strict'; | ||
|
||
/* | ||
the common package gives you some commonly | ||
used testing methods, like mustCall | ||
*/ | ||
const common = require('../common'); | ||
|
||
/* | ||
a small description on what you are testing | ||
*/ | ||
// This test ensures that the http-parser can handle UTF-8 characters | ||
// in the http header. | ||
|
||
const assert = require('assert'); | ||
const http = require('http'); | ||
|
||
/* | ||
the body of the actual test - tests should exit with code 0 on success | ||
*/ | ||
const server = http.createServer( | ||
common.mustCall((request, response) => { | ||
response.write('hello world'); | ||
response.end(); | ||
}) | ||
); | ||
|
||
server.listen(3000, () => {}); | ||
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. This doesn't test anything related to the problem we are trying to solve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this as a global variable will cause concurrent requests to interleave and become corrupt. The buffer needs to be scoped to the response object itself.