debugger: call this.resume() after this.run()#10099
Conversation
This addresses #9854
|
LGTM if CI is ✅ |
cjihrig
left a comment
cjihrig
left a comment
There was a problem hiding this comment.
Makes sense given the commit that introduced the regression. LGTM as far as I can tell.
|
Hmm CI is not happy. But I can't work out what the issue is with linting. Locally I've run |
|
Let's try a new CI: https://ci.nodejs.org/job/node-test-pull-request/5150/ |
| this.pause(); | ||
|
|
||
| setImmediate(() => { this.run(); }); | ||
| setImmediate(() => { this.run( () => this.resume() ); }); |
There was a problem hiding this comment.
Extra spaces between ( and (), and () and ).
|
Do we have a test for this? |
|
@indutny no test yet. Existing tests did not catch the regression, but I haven't worked out a new test. I don't think I'll be able to get to it today, but I can devise something in the next couple of days if necessary. Edit Test added |
|
And yet a 3rd CI https://ci.nodejs.org/job/node-test-pull-request/5151/ |
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
LGTM. Seemingly unrelated flake on the freebsd10-64 buildbot.
| const common = require('../common'); | ||
| const spawn = require('child_process').spawn; | ||
|
|
||
| const timeoutId = setTimeout(function() { |
There was a problem hiding this comment.
Is the timeout needed? Couldn't we just let the test timeout and fail instead of introducing a timer?
There was a problem hiding this comment.
I suppose it is redundant. I was just modeling off of another test that spawns a node process.
There was a problem hiding this comment.
Yea, if you don't need it, I'd remove it. They add complexity to the code and timers have been a source of annoyance on the CI.
|
Landed in 6410534 |
|
This should have had another CI run before landing I think. |
|
@cjihrig sorry about that - I did run another CI but didn't link it. Looks like unrelated flakiness on Windows. https://ci.nodejs.org/job/node-test-pull-request/5210/ |
When the debugger has started we need to call `this.resume` otherwise, the prompt won't appear. Fixes: nodejs#9854 PR-URL: nodejs#10099 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>
6 participants
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
debugger
Description of change
This addresses #9854
The issue was introduced here and first appeared in 6.2.2.
I haven't figured out the best way to test this yet. Apparently, existing tests didn't catch the regression though.