diff --git a/lib/_http_common.js b/lib/_http_common.js index 2055ca205b84a3..4430869520f813 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -117,7 +117,7 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method, return parser.onIncoming(incoming, shouldKeepAlive); } -function parserOnBody(b, start, len) { +function parserOnBody(b) { const stream = this.incoming; // If the stream has already been removed, then drop it. @@ -125,9 +125,8 @@ function parserOnBody(b, start, len) { return; // Pretend this was the result of a stream._read call. - if (len > 0 && !stream._dumped) { - const slice = b.slice(start, start + len); - const ret = stream.push(slice); + if (!stream._dumped) { + const ret = stream.push(b); if (!ret) readStop(this.socket); } diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 7ff5b0fe2ff76f..52c8e9e2589dd4 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -248,11 +248,7 @@ class Parser : public AsyncWrap, public StreamListener { binding_data_(binding_data) { } - - void MemoryInfo(MemoryTracker* tracker) const override { - tracker->TrackField("current_buffer", current_buffer_); - } - + SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(Parser) SET_SELF_SIZE(Parser) @@ -454,32 +450,20 @@ class Parser : public AsyncWrap, public StreamListener { int on_body(const char* at, size_t length) { - EscapableHandleScope scope(env()->isolate()); + if (length == 0) + return 0; - Local obj = object(); - Local cb = obj->Get(env()->context(), kOnBody).ToLocalChecked(); + Environment* env = this->env(); + HandleScope handle_scope(env->isolate()); + + Local cb = object()->Get(env->context(), kOnBody).ToLocalChecked(); if (!cb->IsFunction()) return 0; - // We came from consumed stream - if (current_buffer_.IsEmpty()) { - // Make sure Buffer will be in parent HandleScope - current_buffer_ = scope.Escape(Buffer::Copy( - env()->isolate(), - current_buffer_data_, - current_buffer_len_).ToLocalChecked()); - } + Local buffer = Buffer::Copy(env, at, length).ToLocalChecked(); - Local argv[3] = { - current_buffer_, - Integer::NewFromUnsigned( - env()->isolate(), static_cast(at - current_buffer_data_)), - Integer::NewFromUnsigned(env()->isolate(), length)}; - - MaybeLocal r = MakeCallback(cb.As(), - arraysize(argv), - argv); + MaybeLocal r = MakeCallback(cb.As(), 1, &buffer); if (r.IsEmpty()) { got_exception_ = true; @@ -593,17 +577,9 @@ class Parser : public AsyncWrap, public StreamListener { static void Execute(const FunctionCallbackInfo& args) { Parser* parser; ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); - CHECK(parser->current_buffer_.IsEmpty()); - CHECK_EQ(parser->current_buffer_len_, 0); - CHECK_NULL(parser->current_buffer_data_); ArrayBufferViewContents buffer(args[0]); - // This is a hack to get the current_buffer to the callbacks with the least - // amount of overhead. Nothing else will run while http_parser_execute() - // runs, therefore this pointer can be set and used for the execution. - parser->current_buffer_ = args[0].As(); - Local ret = parser->Execute(buffer.data(), buffer.length()); if (!ret.IsEmpty()) @@ -615,7 +591,6 @@ class Parser : public AsyncWrap, public StreamListener { Parser* parser; ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); - CHECK(parser->current_buffer_.IsEmpty()); Local ret = parser->Execute(nullptr, 0); if (!ret.IsEmpty()) @@ -695,11 +670,6 @@ class Parser : public AsyncWrap, public StreamListener { // Should always be called from the same context. CHECK_EQ(env, parser->env()); - if (parser->execute_depth_) { - parser->pending_pause_ = should_pause; - return; - } - if (should_pause) { llhttp_pause(&parser->parser_); } else { @@ -801,7 +771,6 @@ class Parser : public AsyncWrap, public StreamListener { if (nread == 0) return; - current_buffer_.Clear(); Local ret = Execute(buf.base, nread); // Exception @@ -834,17 +803,12 @@ class Parser : public AsyncWrap, public StreamListener { llhttp_errno_t err; - // Do not allow re-entering `http_parser_execute()` - CHECK_EQ(execute_depth_, 0); - - execute_depth_++; if (data == nullptr) { err = llhttp_finish(&parser_); } else { err = llhttp_execute(&parser_, data, len); Save(); } - execute_depth_--; // Calculate bytes read and resume after Upgrade/CONNECT pause size_t nread = len; @@ -864,8 +828,6 @@ class Parser : public AsyncWrap, public StreamListener { llhttp_pause(&parser_); } - // Unassign the 'buffer_' variable - current_buffer_.Clear(); current_buffer_len_ = 0; current_buffer_data_ = nullptr; @@ -989,8 +951,6 @@ class Parser : public AsyncWrap, public StreamListener { int MaybePause() { - CHECK_NE(execute_depth_, 0); - if (!pending_pause_) { return 0; } @@ -1018,10 +978,8 @@ class Parser : public AsyncWrap, public StreamListener { size_t num_values_; bool have_flushed_; bool got_exception_; - Local current_buffer_; size_t current_buffer_len_; const char* current_buffer_data_; - unsigned int execute_depth_ = 0; bool headers_completed_ = false; bool pending_pause_ = false; uint64_t header_nread_ = 0; diff --git a/test/parallel/test-http-parser-multiple-execute.js b/test/parallel/test-http-parser-multiple-execute.js new file mode 100644 index 00000000000000..d05a973752395a --- /dev/null +++ b/test/parallel/test-http-parser-multiple-execute.js @@ -0,0 +1,37 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { request } = require('http'); +const { Duplex } = require('stream'); + +let socket; + +function createConnection(...args) { + socket = new Duplex({ + read() {}, + write(chunk, encoding, callback) { + if (chunk.toString().includes('\r\n\r\n')) { + this.push('HTTP/1.1 100 Continue\r\n\r\n'); + } + + callback(); + } + }); + + return socket; +} + +const req = request('http://localhost:8080', { createConnection }); + +req.on('information', common.mustCall(({ statusCode }) => { + assert.strictEqual(statusCode, 100); + socket.push('HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n'); + socket.push(null); +})); + +req.on('response', common.mustCall(({ statusCode }) => { + assert.strictEqual(statusCode, 200); +})); + +req.end(); diff --git a/test/parallel/test-http-parser.js b/test/parallel/test-http-parser.js index 97dc57f755ad88..739beaa3d67cd9 100644 --- a/test/parallel/test-http-parser.js +++ b/test/parallel/test-http-parser.js @@ -62,8 +62,8 @@ function newParser(type) { function expectBody(expected) { - return mustCall(function(buf, start, len) { - const body = String(buf.slice(start, start + len)); + return mustCall(function(buf) { + const body = String(buf); assert.strictEqual(body, expected); }); } @@ -126,8 +126,8 @@ function expectBody(expected) { assert.strictEqual(statusMessage, 'OK'); }; - const onBody = (buf, start, len) => { - const body = String(buf.slice(start, start + len)); + const onBody = (buf) => { + const body = String(buf); assert.strictEqual(body, 'pong'); }; @@ -195,8 +195,8 @@ function expectBody(expected) { parser[kOnHeaders] = mustCall(onHeaders); }; - const onBody = (buf, start, len) => { - const body = String(buf.slice(start, start + len)); + const onBody = (buf) => { + const body = String(buf); assert.strictEqual(body, 'ping'); seen_body = true; }; @@ -291,8 +291,8 @@ function expectBody(expected) { assert.strictEqual(versionMinor, 1); }; - const onBody = (buf, start, len) => { - const body = String(buf.slice(start, start + len)); + const onBody = (buf) => { + const body = String(buf); assert.strictEqual(body, 'foo=42&bar=1337'); }; @@ -332,8 +332,8 @@ function expectBody(expected) { let body_part = 0; const body_parts = ['123', '123456', '1234567890']; - const onBody = (buf, start, len) => { - const body = String(buf.slice(start, start + len)); + const onBody = (buf) => { + const body = String(buf); assert.strictEqual(body, body_parts[body_part++]); }; @@ -371,8 +371,8 @@ function expectBody(expected) { const body_parts = ['123', '123456', '123456789', '123456789ABC', '123456789ABCDEF']; - const onBody = (buf, start, len) => { - const body = String(buf.slice(start, start + len)); + const onBody = (buf) => { + const body = String(buf); assert.strictEqual(body, body_parts[body_part++]); }; @@ -428,8 +428,8 @@ function expectBody(expected) { let expected_body = '123123456123456789123456789ABC123456789ABCDEF'; - const onBody = (buf, start, len) => { - const chunk = String(buf.slice(start, start + len)); + const onBody = (buf) => { + const chunk = String(buf); assert.strictEqual(expected_body.indexOf(chunk), 0); expected_body = expected_body.slice(chunk.length); }; @@ -445,9 +445,7 @@ function expectBody(expected) { for (let i = 1; i < request.length - 1; ++i) { const a = request.slice(0, i); - console.error(`request.slice(0, ${i}) = ${JSON.stringify(a.toString())}`); const b = request.slice(i); - console.error(`request.slice(${i}) = ${JSON.stringify(b.toString())}`); test(a, b); } } @@ -488,8 +486,8 @@ function expectBody(expected) { let expected_body = '123123456123456789123456789ABC123456789ABCDEF'; - const onBody = (buf, start, len) => { - const chunk = String(buf.slice(start, start + len)); + const onBody = (buf) => { + const chunk = String(buf); assert.strictEqual(expected_body.indexOf(chunk), 0); expected_body = expected_body.slice(chunk.length); };