Skip to content

Commit ece5073

Browse files
Driegermmarchini
Drieger
authored and
mmarchini
committed
src: do not reuse async resource in http parsers
Change resource being used, previously HTTParser was being reused. We are now using IncomingMessage and ClientRequest objects. The goal here is to make the async resource unique for each async operatio Refs: nodejs#24330 Refs: nodejs/diagnostics#248 Refs: nodejs#21313 Co-authored-by: Matheus Marchini <[email protected]> PR-URL: nodejs#25094 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benedikt Meurer <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
1 parent 5aaf666 commit ece5073

13 files changed

Lines changed: 102 additions & 32 deletions

‎doc/api/async_hooks.md‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -236,10 +236,10 @@ The `type` is a string identifying the type of resource that caused
236236
resource's constructor.
237237

238238
```text
239-
FSEVENTWRAP, FSREQCALLBACK, GETADDRINFOREQWRAP, GETNAMEINFOREQWRAP, HTTPPARSER,
240-
JSSTREAM, PIPECONNECTWRAP, PIPEWRAP, PROCESSWRAP, QUERYWRAP, SHUTDOWNWRAP,
241-
SIGNALWRAP, STATWATCHER, TCPCONNECTWRAP, TCPSERVERWRAP, TCPWRAP, TTYWRAP,
242-
UDPSENDWRAP, UDPWRAP, WRITEWRAP, ZLIB, SSLCONNECTION, PBKDF2REQUEST,
239+
FSEVENTWRAP, FSREQCALLBACK, GETADDRINFOREQWRAP, GETNAMEINFOREQWRAP, HTTPINCOMINGMESSAGE,
240+
HTTPCLIENTREQUEST, JSSTREAM, PIPECONNECTWRAP, PIPEWRAP, PROCESSWRAP, QUERYWRAP,
241+
SHUTDOWNWRAP, SIGNALWRAP, STATWATCHER, TCPCONNECTWRAP, TCPSERVERWRAP, TCPWRAP,
242+
TTYWRAP, UDPSENDWRAP, UDPWRAP, WRITEWRAP, ZLIB, SSLCONNECTION, PBKDF2REQUEST,
243243
RANDOMBYTESREQUEST, TLSWRAP, Microtask, Timeout, Immediate, TickObject
244244
```
245245

‎lib/_http_client.js‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -632,11 +632,10 @@ function emitFreeNT(socket) {
632632
}
633633

634634
function tickOnSocket(req, socket) {
635-
const isParserReused = parsers.hasItems();
636635
const parser = parsers.alloc();
637636
req.socket = socket;
638637
req.connection = socket;
639-
parser.reinitialize(HTTPParser.RESPONSE, isParserReused);
638+
parser.initialize(HTTPParser.RESPONSE, req);
640639
parser.socket = socket;
641640
parser.outgoing = req;
642641
req.parser = parser;

‎lib/_http_server.js‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,12 @@ const STATUS_CODES = {
126126

127127
const kOnExecute = HTTPParser.kOnExecute | 0;
128128

129+
class HTTPServerAsyncResource {
130+
constructor(type, socket) {
131+
this.type = type;
132+
this.socket = socket;
133+
}
134+
}
129135

130136
function ServerResponse(req) {
131137
OutgoingMessage.call(this);
@@ -349,9 +355,15 @@ function connectionListenerInternal(server, socket) {
349355
socket.setTimeout(server.timeout);
350356
socket.on('timeout', socketOnTimeout);
351357

352-
const isParserReused = parsers.hasItems();
353358
const parser = parsers.alloc();
354-
parser.reinitialize(HTTPParser.REQUEST, isParserReused);
359+
360+
// TODO(addaleax): This doesn't play well with the
361+
// `async_hooks.currentResource()` proposal, see
362+
// https://github.com/nodejs/node/pull/21313
363+
parser.initialize(
364+
HTTPParser.REQUEST,
365+
new HTTPServerAsyncResource('HTTPINCOMINGMESSAGE', socket)
366+
);
355367
parser.socket = socket;
356368

357369
// We are starting to wait for our headers.

‎src/async_wrap-inl.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,11 @@ inline AsyncWrap::ProviderType AsyncWrap::provider_type() const {
3434
return provider_type_;
3535
}
3636

37+
inline AsyncWrap::ProviderType AsyncWrap::set_provider_type(
38+
AsyncWrap::ProviderType provider) {
39+
provider_type_ = provider;
40+
return provider_type_;
41+
}
3742

3843
inline double AsyncWrap::get_async_id() const {
3944
return async_id_;

‎src/async_wrap.cc‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -602,11 +602,15 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) {
602602
env->destroy_async_id_list()->push_back(async_id);
603603
}
604604

605+
void AsyncWrap::AsyncReset(double execution_async_id, bool silent) {
606+
AsyncReset(object(), execution_async_id, silent);
607+
}
605608

606609
// Generalized call for both the constructor and for handles that are pooled
607610
// and reused over their lifetime. This way a new uid can be assigned when
608611
// the resource is pulled out of the pool and put back into use.
609-
void AsyncWrap::AsyncReset(double execution_async_id, bool silent) {
612+
void AsyncWrap::AsyncReset(Local<Object> resource, double execution_async_id,
613+
bool silent) {
610614
if (async_id_ != -1) {
611615
// This instance was in use before, we have already emitted an init with
612616
// its previous async_id and need to emit a matching destroy for that
@@ -643,7 +647,7 @@ void AsyncWrap::AsyncReset(double execution_async_id, bool silent) {
643647

644648
if (silent) return;
645649

646-
EmitAsyncInit(env(), object(),
650+
EmitAsyncInit(env(), resource,
647651
env()->async_hooks()->provider_string(provider_type()),
648652
async_id_, trigger_async_id_);
649653
}

‎src/async_wrap.h‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,8 @@ namespace node {
4646
V(HTTP2STREAM) \
4747
V(HTTP2PING) \
4848
V(HTTP2SETTINGS) \
49-
V(HTTPPARSER) \
49+
V(HTTPINCOMINGMESSAGE) \
50+
V(HTTPCLIENTREQUEST) \
5051
V(JSSTREAM) \
5152
V(MESSAGEPORT) \
5253
V(PIPECONNECTWRAP) \
@@ -147,11 +148,16 @@ class AsyncWrap : public BaseObject {
147148
static void DestroyAsyncIdsCallback(Environment* env, void* data);
148149

149150
inline ProviderType provider_type() const;
151+
inline ProviderType set_provider_type(ProviderType provider);
150152

151153
inline double get_async_id() const;
152154

153155
inline double get_trigger_async_id() const;
154156

157+
void AsyncReset(v8::Local<v8::Object> resource,
158+
double execution_async_id = -1,
159+
bool silent = false);
160+
155161
void AsyncReset(double execution_async_id = -1, bool silent = false);
156162

157163
// Only call these within a valid HandleScope.
@@ -202,7 +208,7 @@ class AsyncWrap : public BaseObject {
202208
ProviderType provider,
203209
double execution_async_id,
204210
bool silent);
205-
const ProviderType provider_type_;
211+
ProviderType provider_type_;
206212
// Because the values may be Reset(), cannot be made const.
207213
double async_id_ = -1;
208214
double trigger_async_id_;

‎src/node_http_parser_impl.h‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -152,11 +152,13 @@ struct StringPtr {
152152
size_t size_;
153153
};
154154

155-
156155
class Parser : public AsyncWrap, public StreamListener {
157156
public:
158157
Parser(Environment* env, Local<Object> wrap, parser_type_t type)
159-
: AsyncWrap(env, wrap, AsyncWrap::PROVIDER_HTTPPARSER),
158+
: AsyncWrap(env, wrap,
159+
type == HTTP_REQUEST ?
160+
AsyncWrap::PROVIDER_HTTPINCOMINGMESSAGE :
161+
AsyncWrap::PROVIDER_HTTPCLIENTREQUEST),
160162
current_buffer_len_(0),
161163
current_buffer_data_(nullptr) {
162164
Init(type);
@@ -503,12 +505,12 @@ class Parser : public AsyncWrap, public StreamListener {
503505
}
504506

505507

506-
static void Reinitialize(const FunctionCallbackInfo<Value>& args) {
508+
static void Initialize(const FunctionCallbackInfo<Value>& args) {
507509
Environment* env = Environment::GetCurrent(args);
508510

509511
CHECK(args[0]->IsInt32());
510-
CHECK(args[1]->IsBoolean());
511-
bool isReused = args[1]->IsTrue();
512+
CHECK(args[1]->IsObject());
513+
512514
parser_type_t type =
513515
static_cast<parser_type_t>(args[0].As<Int32>()->Value());
514516

@@ -517,16 +519,16 @@ class Parser : public AsyncWrap, public StreamListener {
517519
ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
518520
// Should always be called from the same context.
519521
CHECK_EQ(env, parser->env());
520-
// This parser has either just been created or it is being reused.
521-
// We must only call AsyncReset for the latter case, because AsyncReset has
522-
// already been called via the constructor for the former case.
523-
if (isReused) {
524-
parser->AsyncReset();
525-
}
522+
523+
AsyncWrap::ProviderType provider =
524+
(type == HTTP_REQUEST ?
525+
AsyncWrap::PROVIDER_HTTPINCOMINGMESSAGE
526+
: AsyncWrap::PROVIDER_HTTPCLIENTREQUEST);
527+
528+
parser->set_provider_type(provider);
526529
parser->Init(type);
527530
}
528531

529-
530532
template <bool should_pause>
531533
static void Pause(const FunctionCallbackInfo<Value>& args) {
532534
Environment* env = Environment::GetCurrent(args);
@@ -958,7 +960,7 @@ void InitializeHttpParser(Local<Object> target,
958960
env->SetProtoMethod(t, "free", Parser::Free);
959961
env->SetProtoMethod(t, "execute", Parser::Execute);
960962
env->SetProtoMethod(t, "finish", Parser::Finish);
961-
env->SetProtoMethod(t, "reinitialize", Parser::Reinitialize);
963+
env->SetProtoMethod(t, "initialize", Parser::Initialize);
962964
env->SetProtoMethod(t, "pause", Parser::Pause<true>);
963965
env->SetProtoMethod(t, "resume", Parser::Pause<false>);
964966
env->SetProtoMethod(t, "consume", Parser::Consume);

‎test/async-hooks/test-httparser-reuse.js‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const http = require('http');
5+
const assert = require('assert');
6+
const { createHook } = require('async_hooks');
7+
const reused = Symbol('reused');
8+
9+
let reusedHTTPParser = false;
10+
const asyncHook = createHook({
11+
init(asyncId, type, triggerAsyncId, resource) {
12+
if (resource[reused]) {
13+
reusedHTTPParser = true;
14+
}
15+
resource[reused] = true;
16+
}
17+
});
18+
asyncHook.enable();
19+
20+
const server = http.createServer(function(req, res) {
21+
res.end();
22+
});
23+
24+
const PORT = 3000;
25+
const url = 'http://127.0.0.1:' + PORT;
26+
27+
server.listen(PORT, common.mustCall(() => {
28+
http.get(url, common.mustCall(() => {
29+
server.close(common.mustCall(() => {
30+
server.listen(PORT, common.mustCall(() => {
31+
http.get(url, common.mustCall(() => {
32+
server.close(common.mustCall(() => {
33+
assert.strictEqual(reusedHTTPParser, false);
34+
}));
35+
}));
36+
}));
37+
}));
38+
}));
39+
}));

‎test/async-hooks/test-httpparser.request.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ const request = Buffer.from(
2121
);
2222

2323
const parser = new HTTPParser(REQUEST);
24-
const as = hooks.activitiesOfTypes('HTTPPARSER');
24+
const as = hooks.activitiesOfTypes('HTTPINCOMINGMESSAGE');
2525
const httpparser = as[0];
2626

2727
assert.strictEqual(as.length, 1);
@@ -47,7 +47,7 @@ process.on('exit', onexit);
4747

4848
function onexit() {
4949
hooks.disable();
50-
hooks.sanityCheck('HTTPPARSER');
50+
hooks.sanityCheck('HTTPINCOMINGMESSAGE');
5151
checkInvocations(httpparser, { init: 1, before: 1, after: 1, destroy: 1 },
5252
'when process exits');
5353
}

‎test/async-hooks/test-httpparser.response.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ const request = Buffer.from(
2626
);
2727

2828
const parser = new HTTPParser(RESPONSE);
29-
const as = hooks.activitiesOfTypes('HTTPPARSER');
29+
const as = hooks.activitiesOfTypes('HTTPCLIENTREQUEST');
3030
const httpparser = as[0];
3131

3232
assert.strictEqual(as.length, 1);
@@ -58,7 +58,7 @@ process.on('exit', onexit);
5858

5959
function onexit() {
6060
hooks.disable();
61-
hooks.sanityCheck('HTTPPARSER');
61+
hooks.sanityCheck('HTTPCLIENTREQUEST');
6262
checkInvocations(httpparser, { init: 1, before: 2, after: 2, destroy: 1 },
6363
'when process exits');
6464
}

0 commit comments

Comments
 (0)