http: improve performance by removing async_hooks #57746
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
|
Pending positive benchmarks, I’m in favor of this change as the http parser does not have inherent asynchronous behavior and it’s all sync. |
src/node_http_parser.cc
Outdated
| env()->context(), object(), 0, nullptr); | ||
|
|
||
| if (r.IsEmpty()) callback_scope.MarkAsFailed(); | ||
| USE(cb.As<Function>()->Call(env()->context(), object(), 0, nullptr)); |
There was a problem hiding this comment.
We should avoid using USE(...) here as it does not allow thrown errors to propagate correctly here. Note the previous code had the check for IsEmpty() and setting MarkAsFailed... that behavior should be retained.
There was a problem hiding this comment.
If there aren't any such tests already, a test verifying that errors thrown in HTTPParser callbacks are handled correctly would be helpful in verifying the changed behavior here.
| if (head_response.IsEmpty()) callback_scope.MarkAsFailed(); | ||
| } | ||
| MaybeLocal<Value> head_response = cb.As<Function>()->Call( | ||
| env()->context(), object(), arraysize(argv), argv); |
There was a problem hiding this comment.
Likewise here, this is not properly handling errors.
| @@ -575,11 +556,6 @@ class Parser : public AsyncWrap, public StreamListener { | |||
| static void Free(const FunctionCallbackInfo<Value>& args) { | |||
| Parser* parser; | |||
| ASSIGN_OR_RETURN_UNWRAP(&parser, args.This()); | |||
|
|
|||
| // Since the Parser destructor isn't going to run the destroy() callbacks | |||
| // it needs to be triggered manually. | |||
There was a problem hiding this comment.
I think we don't need this function anymore.
Pending benchmarks, but this should improve http request performance.
Github search results for HTTPIncomingMessage and HTTPClientRequest