FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http: defer reentrant execution of Parser::Execute · nodejs/node@491c761 · GitHub

/ node Public

Commit 491c761

Browse files
ShogunPanda
authored and
targos
committed
http: defer reentrant execution of Parser::Execute
PR-URL: #43369 Fixes: #39671 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent eece34c commit 491c761

4 files changed

Lines changed: 65 additions & 73 deletions

File tree

‎lib/_http_common.js‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -117,17 +117,16 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method,
117117
return parser.onIncoming(incoming, shouldKeepAlive);
118118
}
119119

120-
function parserOnBody(b, start, len) {
120+
function parserOnBody(b) {
121121
const stream = this.incoming;
122122

123123
// If the stream has already been removed, then drop it.
124124
if (stream === null)
125125
return;
126126

127127
// Pretend this was the result of a stream._read call.
128-
if (len > 0 && !stream._dumped) {
129-
const slice = b.slice(start, start + len);
130-
const ret = stream.push(slice);
128+
if (!stream._dumped) {
129+
const ret = stream.push(b);
131130
if (!ret)
132131
readStop(this.socket);
133132
}

‎src/node_http_parser.cc‎

Lines changed: 9 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -248,11 +248,7 @@ class Parser : public AsyncWrap, public StreamListener {
248248
binding_data_(binding_data) {
249249
}
250250

251-
252-
void MemoryInfo(MemoryTracker* tracker) const override {
253-
tracker->TrackField("current_buffer", current_buffer_);
254-
}
255-
251+
SET_NO_MEMORY_INFO()
256252
SET_MEMORY_INFO_NAME(Parser)
257253
SET_SELF_SIZE(Parser)
258254

@@ -454,32 +450,20 @@ class Parser : public AsyncWrap, public StreamListener {
454450

455451

456452
int on_body(const char* at, size_t length) {
457-
EscapableHandleScope scope(env()->isolate());
453+
if (length == 0)
454+
return 0;
458455

459-
Local<Object> obj = object();
460-
Local<Value> cb = obj->Get(env()->context(), kOnBody).ToLocalChecked();
456+
Environment* env = this->env();
457+
HandleScope handle_scope(env->isolate());
458+
459+
Local<Value> cb = object()->Get(env->context(), kOnBody).ToLocalChecked();
461460

462461
if (!cb->IsFunction())
463462
return 0;
464463

465-
// We came from consumed stream
466-
if (current_buffer_.IsEmpty()) {
467-
// Make sure Buffer will be in parent HandleScope
468-
current_buffer_ = scope.Escape(Buffer::Copy(
469-
env()->isolate(),
470-
current_buffer_data_,
471-
current_buffer_len_).ToLocalChecked());
472-
}
464+
Local<Value> buffer = Buffer::Copy(env, at, length).ToLocalChecked();
473465

474-
Local<Value> argv[3] = {
475-
current_buffer_,
476-
Integer::NewFromUnsigned(
477-
env()->isolate(), static_cast<uint32_t>(at - current_buffer_data_)),
478-
Integer::NewFromUnsigned(env()->isolate(), length)};
479-
480-
MaybeLocal<Value> r = MakeCallback(cb.As<Function>(),
481-
arraysize(argv),
482-
argv);
466+
MaybeLocal<Value> r = MakeCallback(cb.As<Function>(), 1, &buffer);
483467

484468
if (r.IsEmpty()) {
485469
got_exception_ = true;
@@ -593,17 +577,9 @@ class Parser : public AsyncWrap, public StreamListener {
593577
static void Execute(const FunctionCallbackInfo<Value>& args) {
594578
Parser* parser;
595579
ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
596-
CHECK(parser->current_buffer_.IsEmpty());
597-
CHECK_EQ(parser->current_buffer_len_, 0);
598-
CHECK_NULL(parser->current_buffer_data_);
599580

600581
ArrayBufferViewContents<char> buffer(args[0]);
601582

602-
// This is a hack to get the current_buffer to the callbacks with the least
603-
// amount of overhead. Nothing else will run while http_parser_execute()
604-
// runs, therefore this pointer can be set and used for the execution.
605-
parser->current_buffer_ = args[0].As<Object>();
606-
607583
Local<Value> ret = parser->Execute(buffer.data(), buffer.length());
608584

609585
if (!ret.IsEmpty())
@@ -615,7 +591,6 @@ class Parser : public AsyncWrap, public StreamListener {
615591
Parser* parser;
616592
ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
617593

618-
CHECK(parser->current_buffer_.IsEmpty());
619594
Local<Value> ret = parser->Execute(nullptr, 0);
620595

621596
if (!ret.IsEmpty())
@@ -695,11 +670,6 @@ class Parser : public AsyncWrap, public StreamListener {
695670
// Should always be called from the same context.
696671
CHECK_EQ(env, parser->env());
697672

698-
if (parser->execute_depth_) {
699-
parser->pending_pause_ = should_pause;
700-
return;
701-
}
702-
703673
if (should_pause) {
704674
llhttp_pause(&parser->parser_);
705675
} else {
@@ -801,7 +771,6 @@ class Parser : public AsyncWrap, public StreamListener {
801771
if (nread == 0)
802772
return;
803773

804-
current_buffer_.Clear();
805774
Local<Value> ret = Execute(buf.base, nread);
806775

807776
// Exception
@@ -834,17 +803,12 @@ class Parser : public AsyncWrap, public StreamListener {
834803

835804
llhttp_errno_t err;
836805

837-
// Do not allow re-entering `http_parser_execute()`
838-
CHECK_EQ(execute_depth_, 0);
839-
840-
execute_depth_++;
841806
if (data == nullptr) {
842807
err = llhttp_finish(&parser_);
843808
} else {
844809
err = llhttp_execute(&parser_, data, len);
845810
Save();
846811
}
847-
execute_depth_--;
848812

849813
// Calculate bytes read and resume after Upgrade/CONNECT pause
850814
size_t nread = len;
@@ -864,8 +828,6 @@ class Parser : public AsyncWrap, public StreamListener {
864828
llhttp_pause(&parser_);
865829
}
866830

867-
// Unassign the 'buffer_' variable
868-
current_buffer_.Clear();
869831
current_buffer_len_ = 0;
870832
current_buffer_data_ = nullptr;
871833

@@ -989,8 +951,6 @@ class Parser : public AsyncWrap, public StreamListener {
989951

990952

991953
int MaybePause() {
992-
CHECK_NE(execute_depth_, 0);
993-
994954
if (!pending_pause_) {
995955
return 0;
996956
}
@@ -1018,10 +978,8 @@ class Parser : public AsyncWrap, public StreamListener {
1018978
size_t num_values_;
1019979
bool have_flushed_;
1020980
bool got_exception_;
1021-
Local<Object> current_buffer_;
1022981
size_t current_buffer_len_;
1023982
const char* current_buffer_data_;
1024-
unsigned int execute_depth_ = 0;
1025983
bool headers_completed_ = false;
1026984
bool pending_pause_ = false;
1027985
uint64_t header_nread_ = 0;

‎test/parallel/test-http-parser-multiple-execute.js‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const { request } = require('http');
6+
const { Duplex } = require('stream');
7+
8+
let socket;
9+
10+
function createConnection(...args) {
11+
socket = new Duplex({
12+
read() {},
13+
write(chunk, encoding, callback) {
14+
if (chunk.toString().includes('\r\n\r\n')) {
15+
this.push('HTTP/1.1 100 Continue\r\n\r\n');
16+
}
17+
18+
callback();
19+
}
20+
});
21+
22+
return socket;
23+
}
24+
25+
const req = request('http://localhost:8080', { createConnection });
26+
27+
req.on('information', common.mustCall(({ statusCode }) => {
28+
assert.strictEqual(statusCode, 100);
29+
socket.push('HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n');
30+
socket.push(null);
31+
}));
32+
33+
req.on('response', common.mustCall(({ statusCode }) => {
34+
assert.strictEqual(statusCode, 200);
35+
}));
36+
37+
req.end();

‎test/parallel/test-http-parser.js‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,8 @@ function newParser(type) {
6262

6363

6464
function expectBody(expected) {
65-
return mustCall(function(buf, start, len) {
66-
const body = String(buf.slice(start, start + len));
65+
return mustCall(function(buf) {
66+
const body = String(buf);
6767
assert.strictEqual(body, expected);
6868
});
6969
}
@@ -126,8 +126,8 @@ function expectBody(expected) {
126126
assert.strictEqual(statusMessage, 'OK');
127127
};
128128

129-
const onBody = (buf, start, len) => {
130-
const body = String(buf.slice(start, start + len));
129+
const onBody = (buf) => {
130+
const body = String(buf);
131131
assert.strictEqual(body, 'pong');
132132
};
133133

@@ -195,8 +195,8 @@ function expectBody(expected) {
195195
parser[kOnHeaders] = mustCall(onHeaders);
196196
};
197197

198-
const onBody = (buf, start, len) => {
199-
const body = String(buf.slice(start, start + len));
198+
const onBody = (buf) => {
199+
const body = String(buf);
200200
assert.strictEqual(body, 'ping');
201201
seen_body = true;
202202
};
@@ -291,8 +291,8 @@ function expectBody(expected) {
291291
assert.strictEqual(versionMinor, 1);
292292
};
293293

294-
const onBody = (buf, start, len) => {
295-
const body = String(buf.slice(start, start + len));
294+
const onBody = (buf) => {
295+
const body = String(buf);
296296
assert.strictEqual(body, 'foo=42&bar=1337');
297297
};
298298

@@ -332,8 +332,8 @@ function expectBody(expected) {
332332
let body_part = 0;
333333
const body_parts = ['123', '123456', '1234567890'];
334334

335-
const onBody = (buf, start, len) => {
336-
const body = String(buf.slice(start, start + len));
335+
const onBody = (buf) => {
336+
const body = String(buf);
337337
assert.strictEqual(body, body_parts[body_part++]);
338338
};
339339

@@ -371,8 +371,8 @@ function expectBody(expected) {
371371
const body_parts =
372372
['123', '123456', '123456789', '123456789ABC', '123456789ABCDEF'];
373373

374-
const onBody = (buf, start, len) => {
375-
const body = String(buf.slice(start, start + len));
374+
const onBody = (buf) => {
375+
const body = String(buf);
376376
assert.strictEqual(body, body_parts[body_part++]);
377377
};
378378

@@ -428,8 +428,8 @@ function expectBody(expected) {
428428

429429
let expected_body = '123123456123456789123456789ABC123456789ABCDEF';
430430

431-
const onBody = (buf, start, len) => {
432-
const chunk = String(buf.slice(start, start + len));
431+
const onBody = (buf) => {
432+
const chunk = String(buf);
433433
assert.strictEqual(expected_body.indexOf(chunk), 0);
434434
expected_body = expected_body.slice(chunk.length);
435435
};
@@ -445,9 +445,7 @@ function expectBody(expected) {
445445

446446
for (let i = 1; i < request.length - 1; ++i) {
447447
const a = request.slice(0, i);
448-
console.error(`request.slice(0, ${i}) = ${JSON.stringify(a.toString())}`);
449448
const b = request.slice(i);
450-
console.error(`request.slice(${i}) = ${JSON.stringify(b.toString())}`);
451449
test(a, b);
452450
}
453451
}
@@ -488,8 +486,8 @@ function expectBody(expected) {
488486

489487
let expected_body = '123123456123456789123456789ABC123456789ABCDEF';
490488

491-
const onBody = (buf, start, len) => {
492-
const chunk = String(buf.slice(start, start + len));
489+
const onBody = (buf) => {
490+
const chunk = String(buf);
493491
assert.strictEqual(expected_body.indexOf(chunk), 0);
494492
expected_body = expected_body.slice(chunk.length);
495493
};

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL