Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

debugger: call this.resume() after this.run() #10099

Closed
wants to merge 4 commits into from
Closed

debugger: call this.resume() after this.run() #10099

wants to merge 4 commits into from

Conversation

lance
Copy link
Member

@lance lance commented Dec 2, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected 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.

@lance lance added the debugger label Dec 2, 2016
@Trott
Copy link
Member

Trott commented Dec 3, 2016

The debugger, or at least this implementation of it, is going away after version 7 of Node.js and it's been a challenge to write robust debugger tests. So I'd be OK with making an exception to the usual "bug-fixes should have tests" rule on this one.

Although it might be worth checking if this fixes some of the broken tests in test/debugger. Those tests are not run during make test or during CI runs. A lot of them are broken. :-| But if this fixes one or more of them, then hey, the test has already been written! :-D

Sorry for introducing the bug and thanks for fixing my mistake!

@Trott
Copy link
Member

Trott commented Dec 3, 2016

/cc @indutny @cjihrig

@Trott
Copy link
Member

Trott commented Dec 3, 2016

LGTM if CI is ✅

CI: https://ci.nodejs.org/job/node-test-pull-request/5137/

cjihrig
cjihrig approved these changes Dec 3, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense given the commit that introduced the regression. LGTM as far as I can tell.

@lance
Copy link
Member Author

lance commented Dec 3, 2016

Hmm CI is not happy. But I can't work out what the issue is with linting. Locally I've run make lint-ci on several versions of node, but haven't been able to reproduce the problem. The CI logs were not completely clear to me.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 3, 2016

Let's try a new CI: https://ci.nodejs.org/job/node-test-pull-request/5150/

indutny
indutny reviewed Dec 3, 2016
View reviewed changes
lib/_debugger.js
@@ -829,7 +829,7 @@ function Interface(stdin, stdout, args) {
// Run script automatically
this.pause();

setImmediate(() => { this.run(); });
setImmediate(() => { this.run( () => this.resume() ); });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra spaces between ( and (), and () and ).

@indutny
Copy link
Member

indutny commented Dec 3, 2016

Do we have a test for this?

@lance
Copy link
Member Author

lance commented Dec 3, 2016 •
edited
Loading

@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

@lance
Copy link
Member Author

lance commented Dec 3, 2016

And yet a 3rd CI https://ci.nodejs.org/job/node-test-pull-request/5151/

bnoordhuis
bnoordhuis approved these changes Dec 3, 2016
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Seemingly unrelated flake on the freebsd10-64 buildbot.

cjihrig
cjihrig reviewed Dec 5, 2016
View reviewed changes
test/parallel/test-debug-prompt.js
const common = require('../common');
const spawn = require('child_process').spawn;

const timeoutId = setTimeout(function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the timeout needed? Couldn't we just let the test timeout and fail instead of introducing a timer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it is redundant. I was just modeling off of another test that spawns a node process.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lance
Copy link
Member Author

lance commented Dec 5, 2016

Landed in 6410534

@lance lance closed this Dec 5, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Dec 5, 2016

This should have had another CI run before landing I think.

@lance
Copy link
Member Author

lance commented Dec 5, 2016

@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/

Fishrock123 pushed a commit that referenced this pull request Dec 6, 2016
@lance @Fishrock123
debugger: call this.resume() after this.run()
dfa35d6 
When the debugger has started we need to call `this.resume` otherwise,
the prompt won't appear.

Fixes: #9854
PR-URL: #10099
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@lance
Copy link
Member Author

lance commented Dec 7, 2016

I've added the lts-watch-v6.x label. Should this be back ported to v6.x?

jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
@lance
debugger: call this.resume() after this.run()
81ac97d 
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>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@lance @MylesBorins
debugger: call this.resume() after this.run()
5c8881d 
When the debugger has started we need to call `this.resume` otherwise,
the prompt won't appear.

Fixes: #9854
PR-URL: #10099
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@lance @MylesBorins
debugger: call this.resume() after this.run()
772b50e 
When the debugger has started we need to call `this.resume` otherwise,
the prompt won't appear.

Fixes: #9854
PR-URL: #10099
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@lance @MylesBorins
debugger: call this.resume() after this.run()
b825b3c 
When the debugger has started we need to call `this.resume` otherwise,
the prompt won't appear.

Fixes: #9854
PR-URL: #10099
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@lance @MylesBorins
debugger: call this.resume() after this.run()
bde31f7 
When the debugger has started we need to call `this.resume` otherwise,
the prompt won't appear.

Fixes: #9854
PR-URL: #10099
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This was referenced Jan 24, 2017
Merged
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
@lance @MylesBorins
debugger: call this.resume() after this.run()
8bccd9e 
When the debugger has started we need to call `this.resume` otherwise,
the prompt won't appear.

Fixes: #9854
PR-URL: #10099
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
@lance @MylesBorins
debugger: call this.resume() after this.run()
d8b902f 
When the debugger has started we need to call `this.resume` otherwise,
the prompt won't appear.

Fixes: #9854
PR-URL: #10099
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@indutny indutny indutny left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lance @Trott @cjihrig @indutny @bnoordhuis @MylesBorins