8000 async_hooks: fixup do not reuse HTTPParser · nodejs/node@4b3d0d1 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Commit 4b3d0d1

Browse files
Flarnatargos
Flarna
authored and
targos
committed
async_hooks: fixup do not reuse HTTPParser
Fix some issues introduced/not fixed via #25094: * Init hook is not emitted for a reused HTTPParser * HTTPParser was still used as resource in init hook * type used in init hook was always HTTPINCOMINGMESSAGE even for client requests * some tests have not been adapted to new resource names With this change the async hooks init event is emitted during a call to Initialize() as the type and resource object is available at this time. As a result Initialize() must be called now which could be seen as breaking change even HTTPParser is not part of documented API. It was needed to put the ClientRequest instance into a wrapper object instead passing it directly as async resource otherwise test-domain-multi fails. I think this is because adding an EventEmitter to a Domain adds a property 'domain' and the presence of this changes the context propagation in domains. Besides that tests still refering to resource HTTPParser have been updated/improved. Fixes: #27467 Fixes: #26961 Refs: #25094 PR-URL: #27477 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent caab7d4 commit 4b3d0d1

19 files changed

+139
-59
lines changed

‎benchmark/http/bench-parser.js

+3-2
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,14 @@ function main({ len, n }) {
2424
bench.start();
2525
for (var i = 0; i < n; i++) {
2626
parser.execute(header, 0, header.length);
27-
parser.initialize(REQUEST, header);
27+
parser.initialize(REQUEST, {});
2828
}
2929
bench.end(n);
3030
}
3131

3232
function newParser(type) {
33-
const parser = new HTTPParser(type);
33+
const parser = new HTTPParser();
34+
parser.initialize(type, {});
3435

3536
parser.headers = [];
3637

‎lib/_http_client.js

+9-1
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,13 @@ function validateHost(host, name) {
6464
return host;
6565
}
6666

67+
class HTTPClientAsyncResource {
68+
constructor(type, req) {
69+
this.type = type;
70+
this.req = req;
71+
}
72+
}
73+
6774
let urlWarningEmitted = false;
6875
function ClientRequest(input, options, cb) {
6976
OutgoingMessage.call(this);
@@ -635,7 +642,8 @@ function tickOnSocket(req, socket) {
635642
const parser = parsers.alloc();
636643
req.socket = socket;
637644
req.connection = socket;
638-
parser.initialize(HTTPParser.RESPONSE, req);
645+
parser.initialize(HTTPParser.RESPONSE,
646+
new HTTPClientAsyncResource('HTTPINCOMINGMESSAGE', req));
639647
parser.socket = socket;
640648
parser.outgoing = req;
641649
req.parser = parser;

‎lib/_http_common.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ function parserOnMessageComplete() {
156156

157157

158158
const parsers = new FreeList('parsers', 1000, function parsersCb() {
159-
const parser = new HTTPParser(HTTPParser.REQUEST);
159+
const parser = new HTTPParser();
160160

161161
cleanParser(parser);
162162

‎src/async_wrap.cc

+22-9
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) {
178178
class PromiseWrap : public AsyncWrap {
179179
public:
180180
PromiseWrap(Environment* env, Local<Object> object, bool silent)
181-
: AsyncWrap(env, object, PROVIDER_PROMISE, -1, silent) {
181+
: AsyncWrap(env, object, PROVIDER_PROMISE, kInvalidAsyncId, silent) {
182182
MakeWeak();
183183
}
184184

@@ -388,7 +388,7 @@ static void RegisterDestroyHook(const FunctionCallbackInfo<Value>& args) {
388388

389389
void AsyncWrap::GetAsyncId(const FunctionCallbackInfo<Value>& args) {
390390
AsyncWrap* wrap;
391-
args.GetReturnValue().Set(-1);
391+
args.GetReturnValue().Set(kInvalidAsyncId);
392392
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
393393
args.GetReturnValue().Set(wrap->get_async_id());
394394
}
@@ -415,10 +415,15 @@ void AsyncWrap::AsyncReset(const FunctionCallbackInfo<Value>& args) {
415415
AsyncWrap* wrap;
416416
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
417417
double execution_async_id =
418-
args[0]->IsNumber() ? args[0].As<Number>()->Value() : -1;
418+
args[0]->IsNumber() ? args[0].As<Number>()->Value() : kInvalidAsyncId;
419419
wrap->AsyncReset(execution_async_id);
420420
}
421421

422+
void AsyncWrap::EmitDestroy() {
423+
AsyncWrap::EmitDestroy(env(), async_id_);
424+
// Ensure no double destroy is emitted via AsyncReset().
425+
async_id_ = kInvalidAsyncId;
426+
}
422427

423428
void AsyncWrap::QueueDestroyAsyncId(const FunctionCallbackInfo<Value>& args) {
424429
CHECK(args[0]->IsNumber());
@@ -481,7 +486,7 @@ void AsyncWrap::Initialize(Local<Object> target,
481486
// kDefaultTriggerAsyncId: Write the id of the resource responsible for a
482487
// handle's creation just before calling the new handle's constructor.
483488
// After the new handle is constructed kDefaultTriggerAsyncId is set back
484-
// to -1.
489+
// to kInvalidAsyncId.
485490
FORCE_SET_TARGET_FIELD(target,
486491
"async_id_fields",
487492
env->async_hooks()->async_id_fields().GetJSArray());
@@ -569,10 +574,16 @@ AsyncWrap::AsyncWrap(Environment* env,
569574
AsyncReset(execution_async_id, silent);
570575
}
571576

577+
AsyncWrap::AsyncWrap(Environment* env, v8::Local<v8::Object> object)
578+
: BaseObject(env, object),
579+
provider_type_(PROVIDER_NONE) {
580+
CHECK_GE(object->InternalFieldCount(), 1);
581+
}
582+
572583

573584
AsyncWrap::~AsyncWrap() {
574585
EmitTraceEventDestroy();
575-
EmitDestroy(env(), get_async_id());
586+
EmitDestroy();
576587
}
577588

578589
void AsyncWrap::EmitTraceEventDestroy() {
@@ -612,16 +623,18 @@ void AsyncWrap::AsyncReset(double execution_async_id, bool silent) {
612623
// the resource is pulled out of the pool and put back into use.
613624
void AsyncWrap::AsyncReset(Local<Object> resource, double execution_async_id,
614625
bool silent) {
615-
if (async_id_ != -1) {
626+
CHECK_NE(provider_type(), PROVIDER_NONE);
627+
628+
if (async_id_ != kInvalidAsyncId) {
616629
// This instance was in use before, we have already emitted an init with
617630
// its previous async_id and need to emit a matching destroy for that
618631
// before generating a new async_id.
619-
EmitDestroy(env(), async_id_);
632+
EmitDestroy();
620633
}
621634

622635
// Now we can assign a new async_id_ to this instance.
623-
async_id_ =
624-
execution_async_id == -1 ? env()->new_async_id() : execution_async_id;
636+
async_id_ = execution_async_id == kInvalidAsyncId ? env()->new_async_id()
637+
: execution_async_id;
625638
trigger_async_id_ = env()->get_default_trigger_async_id();
626639

627640
switch (provider_type()) {

‎src/async_wrap.h

+13-4
Original file line numberDiff line numberDiff line change
@@ -109,12 +109,18 @@ class AsyncWrap : public BaseObject {
109109
AsyncWrap(Environment* env,
110110
v8::Local<v8::Object> object,
111111
ProviderType provider,
112-
double execution_async_id = -1);
112+
double execution_async_id = kInvalidAsyncId);
113+
114+
// This constructor creates a reuseable instance where user is responsible
115+
// to call set_provider_type() and AsyncReset() before use.
116+
AsyncWrap(Environment* env, v8::Local<v8::Object> object);
113117

114118
~AsyncWrap() override;
115119

116120
AsyncWrap() = delete;
117121

122+
static constexpr double kInvalidAsyncId = -1;
123+
118124
static v8::Local<v8::FunctionTemplate> GetConstructorTemplate(
119125
Environment* env);
120126

@@ -141,6 +147,8 @@ class AsyncWrap : public BaseObject {
141147
static void EmitAfter(Environment* env, double async_id);
142148
static void EmitPromiseResolve(Environment* env, double async_id);
143149

150+
void EmitDestroy();
151+
144152
void EmitTraceEventBefore();
145153
static void EmitTraceEventAfter(ProviderType type, double async_id);
146154
void EmitTraceEventDestroy();
@@ -155,10 +163,11 @@ class AsyncWrap : public BaseObject {
155163
inline double get_trigger_async_id() const;
156164

157165
void AsyncReset(v8::Local<v8::Object> resource,
158-
double execution_async_id = -1,
166+
double execution_async_id = kInvalidAsyncId,
159167
bool silent = false);
160168

161-
void AsyncReset(double execution_async_id = -1, bool silent = false);
169+
void AsyncReset(double execution_async_id = kInvalidAsyncId,
170+
bool silent = false);
162171

163172
// Only call these within a valid HandleScope.
164173
v8::MaybeLocal<v8::Value> MakeCallback(const v8::Local<v8::Function> cb,
@@ -210,7 +219,7 @@ class AsyncWrap : public BaseObject {
210219
bool silent);
211220
ProviderType provider_type_;
212221
// Because the values may be Reset(), cannot be made const.
213-
double async_id_ = -1;
222+
double async_id_ = kInvalidAsyncId;
214223
double trigger_async_id_;
215224
};
216225

‎src/node_http_parser_impl.h

+5-13
Original file line numberDiff line numberDiff line change
@@ -154,14 +154,10 @@ struct StringPtr {
154154

155155
class Parser : public AsyncWrap, public StreamListener {
156156
public:
157-
Parser(Environment* env, Local<Object> wrap, parser_type_t type)
158-
: AsyncWrap(env, wrap,
159-
type == HTTP_REQUEST ?
160-
AsyncWrap::PROVIDER_HTTPINCOMINGMESSAGE :
161-
AsyncWrap::PROVIDER_HTTPCLIENTREQUEST),
157+
Parser(Environment* env, Local<Object> wrap)
158+
: AsyncWrap(env, wrap),
162159
current_buffer_len_(0),
163160
current_buffer_data_(nullptr) {
164-
Init(type);
165161
}
166162

167163

@@ -426,11 +422,7 @@ class Parser : public AsyncWrap, public StreamListener {
426422

427423
static void New(const FunctionCallbackInfo<Value>& args) {
428424
Environment* env = Environment::GetCurrent(args);
429-
CHECK(args[0]->IsInt32());
430-
parser_type_t type =
431-
static_cast<parser_type_t>(args[0].As<Int32>()->Value());
432-
CHECK(type == HTTP_REQUEST || type == HTTP_RESPONSE);
433-
new Parser(env, args.This(), type);
425+
new Parser(env, args.This());
434426
}
435427

436428

@@ -443,14 +435,13 @@ class Parser : public AsyncWrap, public StreamListener {
443435

444436

445437
static void Free(const FunctionCallbackInfo<Value>& args) {
446-
Environment* env = Environment::GetCurrent(args);
447438
Parser* parser;
448439
ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
449440

450441
// Since the Parser destructor isn't going to run the destroy() callbacks
451442
// it needs to be triggered manually.
452443
parser->EmitTraceEventDestroy();
453-
parser->EmitDestroy(env, parser->get_async_id());
444+
parser->EmitDestroy();
454445
}
455446

456447

@@ -526,6 +517,7 @@ class Parser : public AsyncWrap, public StreamListener {
526517
: AsyncWrap::PROVIDER_HTTPCLIENTREQUEST);
527518

528519
parser->set_provider_type(provider);
520+
parser->AsyncReset(args[1].As<Object>());
529521
parser->Init(type);
530522
}
531523

‎test/async-hooks/coverage.md

+2-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@ Showing which kind of async resource is covered by which test:
99
| FSREQCALLBACK | test-fsreqcallback-{access,readFile}.js |
1010
| GETADDRINFOREQWRAP | test-getaddrinforeqwrap.js |
1111
| GETNAMEINFOREQWRAP | test-getnameinforeqwrap.js |
12-
| HTTPPARSER | test-httpparser.{request,response}.js |
12+
| HTTPINCOMINGMESSAGE | test-httpparser.request.js |
13+
| HTTPCLIENTREQUEST | test-httpparser.response.js |
1314
| Immediate | test-immediate.js |
1415
| JSSTREAM | TODO (crashes when accessing directly) |
1516
| PBKDF2REQUEST | test-crypto-pbkdf2.js |

‎test/async-hooks/test-graph.http.js

+4-4
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,13 @@ process.on('exit', function() {
3535
{ type: 'TCPCONNECTWRAP',
3636
id: 'tcpconnect:1',
3737
triggerAsyncId: 'tcp:1' },
38-
{ type: 'HTTPPARSER',
39-
id: 'httpparser:1',
38+
{ type: 'HTTPCLIENTREQUEST',
39+
id: 'httpclientrequest:1',
4040
triggerAsyncId: 'tcpserver:1' },
4141
{ type: 'TCPWRAP', id: 'tcp:2', triggerAsyncId: 'tcpserver:1' },
4242
{ type: 'Timeout', id: 'timeout:1', triggerAsyncId: 'tcp:2' },
43-
{ type: 'HTTPPARSER',
44-
id: 'httpparser:2',
43+
{ type: 'HTTPINCOMINGMESSAGE',
44+
id: 'httpincomingmessage:1',
4545
triggerAsyncId: 'tcp:2' },
4646
{ type: 'Timeout',
4747
id: 'timeout:2',

‎test/async-hooks/test-graph.tls-write.js

-4
Original file line numberDiff line numberDiff line change
@@ -64,12 +64,8 @@ function onexit() {
6464
id: 'getaddrinforeq:1', triggerAsyncId: 'tls:1' },
6565
{ type: 'TCPCONNECTWRAP',
6666
id: 'tcpconnect:1', triggerAsyncId: 'tcp:1' },
67-
{ type: 'WRITEWRAP', id: 'write:1', triggerAsyncId: 'tcpconnect:1' },
6867
{ type: 'TCPWRAP', id: 'tcp:2', triggerAsyncId: 'tcpserver:1' },
6968
{ type: 'TLSWRAP', id: 'tls:2', triggerAsyncId: 'tcpserver:1' },
70-
{ type: 'WRITEWRAP', id: 'write:2', triggerAsyncId: null },
71-
{ type: 'WRITEWRAP', id: 'write:3', triggerAsyncId: null },
72-
{ type: 'WRITEWRAP', id: 'write:4', triggerAsyncId: null },
7369
{ type: 'Immediate', id: 'immediate:1', triggerAsyncId: 'tcp:2' },
7470
{ type: 'Immediate', id: 'immediate:2', triggerAsyncId: 'tcp:1' },
7571
]

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

+46-9
Original file line numberDiff line numberDiff line change
@@ -1,39 +1,76 @@
11
'use strict';
22

33
const common = require('../common');
4-
const http = require('http');
54
const assert = require('assert');
65
const { createHook } = require('async_hooks');
6+
const http = require('http');
7+
8+
// Verify that resource emitted for an HTTPParser is not reused.
9+
// Verify that correct create/destroy events are emitted.
10+
711
const reused = Symbol('reused');
812

9-
let reusedHTTPParser = false;
10-
const asyncHook = createHook({
13+
const reusedParser = [];
14+
const incomingMessageParser = [];
15+
const clientRequestParser = [];
16+
const dupDestroys = [];
17+
const destroyed = [];
18+
19+
createHook({
1120
init(asyncId, type, triggerAsyncId, resource) {
21+
switch (type) {
22+
case 'HTTPINCOMINGMESSAGE':
23+
incomingMessageParser.push(asyncId);
24+
break;
25+
case 'HTTPCLIENTREQUEST':
26+
clientRequestParser.push(asyncId);
27+
break;
28+
}
29+
1230
if (resource[reused]) {
13-
reusedHTTPParser = true;
31+
reusedParser.push(
32+
`resource reused: ${asyncId}, ${triggerAsyncId}, ${type}`
33+
);
1434
}
1535
resource[reused] = true;
36+
},
37+
destroy(asyncId) {
38+
if (destroyed.includes(asyncId)) {
39+
dupDestroys.push(asyncId);
40+
} else {
41+
destroyed.push(asyncId);
42+
}
1643
}
17-
});
18-
asyncHook.enable();
44+
}).enable();
1945

20-
const server = http.createServer(function(req, res) {
46+
const server = http.createServer((req, res) => {
2147
res.end();
2248
});
2349

2450
const PORT = 3000;
25-
const url = 'http://127.0.0.1:' + PORT;
51+
const url = `http://127.0.0.1:${PORT}`;
2652

2753
server.listen(PORT, common.mustCall(() => {
2854
http.get(url, common.mustCall(() => {
2955
server.close(common.mustCall(() => {
3056
server.listen(PORT, common.mustCall(() => {
3157
http.get(url, common.mustCall(() => {
3258
server.close(common.mustCall(() => {
33-
assert.strictEqual(reusedHTTPParser, false);
59+
setTimeout(common.mustCall(verify), 200);
3460
}));
3561
}));
3662
}));
3763
}));
3864
}));
3965
}));
66+
67+
function verify() {
68+
assert.strictEqual(reusedParser.length, 0);
69+
70+
assert.strictEqual(incomingMessageParser.length, 2);
71+
assert.strictEqual(clientRequestParser.length, 2);
72+
73+
assert.strictEqual(dupDestroys.length, 0);
74+
incomingMessageParser.forEach((id) => assert.ok(destroyed.includes(id)));
75+
clientRequestParser.forEach((id) => assert.ok(destroyed.includes(id)));
76+
}

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

+2-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@ const request = Buffer.from(
2020
'GET /hello HTTP/1.1\r\n\r\n'
2121
);
2222

23-
const parser = new HTTPParser(REQUEST);
23+
const parser = new HTTPParser();
24+
parser.initialize(REQUEST, {});
2425
const as = hooks.activitiesOfTypes('HTTPINCOMINGMESSAGE');
2526
const httpparser = as[0];
2627

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

+2-1
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@ const request = Buffer.from(
2525
'pong'
2626
);
2727

28-
const parser = new HTTPParser(RESPONSE);
28+
const parser = new HTTPParser();
29+
parser.initialize(RESPONSE, {});
2930
const as = hooks.activitiesOfTypes('HTTPCLIENTREQUEST');
3031
const httpparser = as[0];
3132

0 commit comments

Comments
 (0)
0