inspector,http: support builtin http request bodies#62915
inspector,http: support builtin http request bodies#62915GrinZero wants to merge 18 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
metcoder95
left a comment
There was a problem hiding this comment.
lgtm, it just seems that is gonna need a documentation for the new dcs
|
Thanks for the feedback. Iβve updated the documentation in Specifically, I added/updated entries for:
I also aligned the wording and structure with the existing Node.js docs style for built-in channels. |
|
A slight push, hoping that a maintainer will notice. |
|
Sorry for the ping, @legendecas If you have some time, could you please help review this PR and trigger CI? This PR fixes an issue where the network inspector does not show Thanks a lot! |
|
push, request-ci plz |
|
Ill try to review the PR later, I think it's important we make sure it doesn't leak memory or keep the body buffer reference alive, which can exhaust resource quickly @ShogunPanda ptal |
I'll go explore the issue you mentioned |
|
Thanks for raising this concern. I did a focused memory/lifetime check for this PR, mainly around two questions:
Based on the current implementation and local testing, I do not see evidence that this PR keeps the original JS buffers alive. For both request and response bodies, the payload bytes are copied into inspector-owned storage before being retained by So the inspector is retaining copied payload bytes, not the original userland Buffer objects. I also ran a repeated-request memory test with explicit |
|
The test plan above was proposed by AI, and I reviewed it. For the first test, it uses The second test is a stress test. It lowers |
ab906d5 to
194a5fd
Compare
|
I merged the main branch code to avoid CI errors. |
194a5fd to
ba8f093
Compare
Thanks π I think we should add the request-ci tag to trigger the next steps. I have synced the code of the main branch |
aace601 to
16da9f4
Compare
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62915 +/- ##
==========================================
+ Coverage 89.67% 89.70% +0.03%
==========================================
Files 712 712
Lines 221256 221492 +236
Branches 42397 42443 +46
==========================================
+ Hits 198404 198694 +290
+ Misses 14676 14628 -48
+ Partials 8176 8170 -6
π New features to boost your workflow:
|
Signed-off-by: GrinZero <774933704@qq.com>
c0db72d to
7edffdf
Compare
|
What are the commits after Root cause
hasPostData: !request.writableEndedAt that moment, The fixDefer the
A Why the C++ side has to changeThe previous .setStack(v8_inspector_->captureStackTrace(true)->buildInspectorObject(0))That relied on an implicit invariant: when JS calls Once the emission is deferred, the trigger point becomes a The only correct fix is to capture the stack on the JS side at request-creation time via That in turn requires the C++ backend to accept a JS-supplied initiator object, which is why:
How the follow-up commits fit in
|

Summary
This PR adds builtin
http/httpsrequest-body support to network inspection, soNetwork.getRequestPostDatacan return text request bodies while preserving the existing rejection behavior for binary request bodies.It also moves builtin
httpresponse-body tracking to a raw-byte hook beforeIncomingMessagedecoding, so response inspection remains correct even when user code callsresponse.setEncoding(...).In addition, this PR extends
requestWillBeSentto accept initiator data from JS diagnostics payloads and validates structuredinitiator.stackobjects against the inspector protocol schema before forwarding them to DevTools.Problem
Builtin
http/httpsnetwork inspection currently emits:Network.requestWillBeSentNetwork.responseReceivedNetwork.loadingFinishedHowever, the builtin
httpclient path does not currently expose request-body data to the inspector. As a result,Network.getRequestPostDatacannot return POST data for builtinhttp/httpsrequests.There are two related gaps as well:
IncomingMessage'data'events are not a stable source of raw bytes. If user code callsresponse.setEncoding('utf8'), the chunks observed by userland become strings, while the inspector protocol expects byte-oriented payloads.requestWillBeSentneeds to accept JS-provided initiator data, but structured stack trace input must be validated before it is forwarded as protocol data.Approach
1. Reuse the existing request buffering pipeline
This change does not alter the CDP schema or the existing buffering behavior in
NetworkAgent.Instead, it reuses the current pipeline already used by other transports:
2. Add builtin
httprequest-body diagnostics eventsThe builtin
httpclient now publishes:http.client.request.bodyChunkSenthttp.client.request.bodySentThese events are emitted from the
ClientRequestwrite path before HTTP framing is applied, so the inspector sees the original user payload rather than chunked-transfer framing bytes.3. Capture builtin
httpresponse bytes before decodingFor responses, this PR avoids relying on
IncomingMessage.on('data')innetwork_http.js.Instead, it adds:
http.client.response.bodyChunkReceivedThis hook runs before
Readable.setEncoding()transforms chunks for userland, so the inspector always receives raw bytes.4. Support and validate network initiator payloads
Network.requestWillBeSentcan now use JS diagnostics payloads to provide initiator data.On the C++ side, the incoming JS object is converted into inspector protocol values, and
initiator.stackis validated against theRuntime.StackTraceschema before being attached to the event. If the initiator payload is malformed, the event is rejected with a clear error instead of forwarding bad protocol data to the frontend.If JS does not provide an initiator, the existing behavior remains unchanged: the inspector captures the current stack trace and emits a default
scriptinitiator.5. Document the new diagnostics channels
This PR also updates
diagnostics_channeldocs to cover the new builtin HTTP client request/response body channels and their message shapes.Behavior
After this change:
httpandhttpsPOST requests with UTF-8 text bodies are available throughNetwork.getRequestPostDatahttpresponse inspection continues to work even if user code callsresponse.setEncoding('utf8')Network.requestWillBeSentcan carry JS-provided initiator stack dataTests
This PR adds and extends coverage in:
test/parallel/test-diagnostics-channel-http.jstest/parallel/test-inspector-network-http.jstest/parallel/test-inspector-network-arbitrary-data.jstest/parallel/test-inspector-emit-protocol-event-errors.jsThe updated tests cover:
write()andend()httpandhttpsNetwork.getRequestPostDataresponse.setEncoding('utf8')Verification
Verified locally with:
Refs