FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

debugger: call `this.resume()` after `this.run()` by lance · Pull Request #10099 · nodejs/node · GitHub

/ node Public

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

Closed
lance wants to merge 4 commits into
nodejs:masterfrom
lance:debugger-9854
ConversationCommits (4)ChecksFiles changed
Closed

debugger: call this.resume() after this.run()#10099
lance wants to merge 4 commits into
nodejs:masterfrom
lance:debugger-9854

Conversation

lance commented Dec 2, 2016

Copy link
Copy Markdown
Member
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 added the debugger label Dec 2, 2016

Trott commented Dec 3, 2016

Copy link
Copy Markdown
Member

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 commented Dec 3, 2016

Copy link
Copy Markdown
Member

/cc @indutny @cjihrig

Trott commented Dec 3, 2016

Copy link
Copy Markdown
Member

LGTM if CI is ✅

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

cjihrig approved these changes Dec 3, 2016
View reviewed changes

cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lance commented Dec 3, 2016

Copy link
Copy Markdown
Member Author

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 commented Dec 3, 2016

Copy link
Copy Markdown
Contributor

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

indutny reviewed Dec 3, 2016
View reviewed changes
Comment thread lib/_debugger.js Outdated
this.pause();

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

indutny Dec 3, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

indutny commented Dec 3, 2016

Copy link
Copy Markdown
Member

Do we have a test for this?

lance commented Dec 3, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

@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 commented Dec 3, 2016

Copy link
Copy Markdown
Member Author

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

bnoordhuis approved these changes Dec 3, 2016
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

const timeoutId = setTimeout(function() {

cjihrig Dec 5, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lance Dec 5, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

cjihrig Dec 5, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Dec 5, 2016

Copy link
Copy Markdown
Member Author

Landed in 6410534

lance closed this Dec 5, 2016

cjihrig commented Dec 5, 2016

Copy link
Copy Markdown
Contributor

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

lance commented Dec 5, 2016

Copy link
Copy Markdown
Member Author

@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
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 commented Dec 7, 2016

Copy link
Copy Markdown
Member Author

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
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
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
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
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
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
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cjihrig cjihrig approved these changes
bnoordhuis bnoordhuis approved these changes
+1 more reviewer
indutny indutny left review comments
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL