inspector: Put connect and disconnect events into message queue #7271
Conversation
nodejs-github-bot
added
the
c++
|
Please note that this PR needs fixes uploaded in #7268 - hence the 2 commits. |
mscdex
added
inspector
|
@ofrobots Done. Thanks! |
|
@eugeneo Can you rebase? Thanks. |
|
I rebased the change. Thanks! |
| @@ -170,24 +173,28 @@ class AgentImpl { | |||
| void WaitForDisconnect(); | |||
|
|
|||
| private: | |||
| using MessageQueue = | |||
| std::vector<std::pair<int, const blink::protocol::String16>>; | |||
There was a problem hiding this comment.
Just a suggestion but you could add a using blink::protocol::String16; at the top.
EDIT: I guess it already exists somewhere? I see that line 197 uses a non-qualified name.
There was a problem hiding this comment.
Done. String16STD.h already imports it into the default namespace - so I'll be using it.
|
@bnoordhuis Please take another look. I addressed the comments and rebased the change. Thanks! |
| v8::Isolate* isolate = parent_env_->isolate(); | ||
| platform_->CallOnForegroundThread(isolate, | ||
| new DispatchOnInspectorBackendTask(this)); | ||
| isolate ->RequestInterrupt(InterruptCallback, this); |
There was a problem hiding this comment.
Yep, artifact of line joining. Fixed/reuploaded. Thanks!
|
@bnoordhuis Thanks for the review. I updated the code. |
|
LGTM |
Current implementation tracks connected/disconnected status separately which potentially introduces race condition. This change introduces notion of session IDs and also posts connect/disconnect events into the same queue as the messages. This way Node knows what session given response belongs to and can discard messages if the frontend for that session had disconnected. This also fixes an issue when frontend was unable to attach to V8 instance that was running infinite loop.
|
@ofrobots I updated the CL with a fix for the libc++ (Mac/BSD build issue). The only change was that MessageQueue vector now stores String16 and not cons String16. |
|
New CI: https://ci.nodejs.org/job/node-test-pull-request/3156/. Looks green. |
|
Apologies for the delay. Since it has been ~2 weeks since the last CI, I reran a new one here: https://ci.nodejs.org/job/node-test-pull-request/3299/. |
|
Looks green modulo arm-fanned jobs which seems to be a jenkins slave issue. |
Current implementation tracks connected/disconnected status separately which potentially introduces race condition. This change introduces notion of session IDs and also posts connect/disconnect events into the same queue as the messages. This way Node knows what session given response belongs to and can discard messages if the frontend for that session had disconnected. This also fixes an issue when frontend was unable to attach to V8 instance that was running infinite loop. PR-URL: #7271 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
|
Landed as 4220c24. |
Current implementation tracks connected/disconnected status separately which potentially introduces race condition. This change introduces notion of session IDs and also posts connect/disconnect events into the same queue as the messages. This way Node knows what session given response belongs to and can discard messages if the frontend for that session had disconnected. This also fixes an issue when frontend was unable to attach to V8 instance that was running infinite loop. PR-URL: #7271 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Current implementation tracks connected/disconnected status separately which potentially introduces race condition. This change introduces notion of session IDs and also posts connect/disconnect events into the same queue as the messages. This way Node knows what session given response belongs to and can discard messages if the frontend for that session had disconnected. This also fixes an issue when frontend was unable to attach to V8 instance that was running infinite loop. PR-URL: #7271 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Checklist
make -j4 test(UNIX) orvcbuild test nosign(Windows) passesAffected core subsystem(s)
inspector
Description of change
Putting connect and disconnect events into the message queue removes some unlikely race conditions. It also allows reusing the V8 interrupt when frontend connects to Node.js running an infinite loop.
CC: @ofrobots