Conversation
|
Review requested:
|
d12ac94 to
2263e95
Compare
|
@nodejs/diagnostics ptal. Not sure if anyone is monkeypatching those callbacks on the fly. |
2263e95 to
781531a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66152 +/- ##
=======================================
Coverage 90.37% 90.37%
=======================================
Files 790 790
Lines 274514 274526 +12
Branches 52572 52576 +4
=======================================
+ Hits 248102 248115 +13
+ Misses 16889 16884 -5
- Partials 9523 9527 +4
🚀 New features to boost your workflow:
|
781531a to
eea709d
Compare
| } | ||
|
|
||
| // Caches a set-once callback property; the cache is cleared in Init(). | ||
| Local<Value> CachedCallback(uint32_t index, v8::Global<v8::Value>* cache) { |
There was a problem hiding this comment.
| Local<Value> CachedCallback(uint32_t index, v8::Global<v8::Value>* cache) { | |
| Local<Value> CachedCallback(uint32_t index, v8::Global<v8::Value>& cache) { |
maybe?
And change -> by . to access it.
| llhttp_init(&parser_, type, &settings); | ||
|
|
||
| on_headers_complete_cb_.Reset(); | ||
| on_body_cb_.Reset(); |
There was a problem hiding this comment.
should this be done earlier, e.g. when parser is moved into freelist on managed side instead on next use?
|
|
||
| v8::Global<v8::Value> on_headers_complete_cb_; | ||
| v8::Global<v8::Value> on_body_cb_; | ||
| v8::Global<v8::Value> on_message_complete_cb_; |
There was a problem hiding this comment.
Is there a reason why kOnHeaders callback is not cached?
Are the two complete callbacks called more then once per HTTP request? I assume caching for single use would be not helpful.
There was a problem hiding this comment.
v8::Globalreferring to callbacks is a memory leak footgun, as any reference from the callback back to the parser will create an indefinite memory leak- If we do use
v8::Globals, they need to be tracked in theParserclass's MemoryInfo method
If we do want to cache these methods, internal fields are likely a better choice (or, as a secondary option, weak v8::Globals, but I'd carefully evaluate the performance of that approach)
|
This pull request has conflicts with its base branch, removing the |
Cache the per-message callback lookups on the parser's JS object instead of retaining callback functions in strong v8::Global handles. A callback that captures its parser can otherwise keep the parser alive. Clear the cache when a parser is initialized or freed so reused parsers can load replacement callbacks and idle parsers do not retain them. Header field names remain non-internalized because they are supplied by clients. Assisted-by: pi Signed-off-by: Matteo Collina <hello@matteocollina.com>
eea709d to
2504fd1
Compare
Cache the
kOnHeadersComplete,kOnBody, andkOnMessageCompletelookups in internal fields on each parser’s JS object, rather than in strongv8::Globalhandles. This avoids an independent strong reference when a callback captures its parser.The cache is cleared when the parser is initialized or freed. Callbacks replaced while a parser is active are picked up on its next initialization. Header names remain non-internalized because clients control them.
The new test covers repeated requests, callback replacement on reinitialization, and release of a cached callback on
free(). The Release build, lint, formatting check, and focused tests passed. In the broader HTTP run, 500 tests passed and one timed out; the timed-out test passed when rerun alone.