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

Revert "src: fix stuck debugger process" by bnoordhuis · Pull Request #3585 · nodejs/node · GitHub

/ node Public

Revert "src: fix stuck debugger process" - #3585

Merged
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:revert-pr2778
Oct 29, 2015
ConversationCommits (2)ChecksFiles changed
Merged

Revert "src: fix stuck debugger process"#3585
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:revert-pr2778

Conversation

bnoordhuis commented Oct 29, 2015

Copy link
Copy Markdown
Member

This was referenced Oct 29, 2015
Merged
Closed

evanlucas commented Oct 29, 2015

Copy link
Copy Markdown
Contributor

@bnoordhuis mind explaining the revert and where the regression is?

bnoordhuis commented Oct 29, 2015

Copy link
Copy Markdown
Member Author

It's in the commit log, let me know if it's not clear enough:

Reverted for breaking node --debug-brk -e 0. It should immediately quit but instead it hangs now.

The regression is that the uv_async_t debug handle is not unref'd when --debug-brk is specified. I want to revert first instead of trying to fix it because the commit is lined up for the next LTS release.

jasnell commented Oct 29, 2015

Copy link
Copy Markdown
Member

Yep, thanks for catching that @bnoordhuis. Fortunately the commit is only in staging and hasn't yet been landed in the v4.x branch so I'll simply pull it from the list of commits that I land in v4.x today.

evanlucas commented Oct 29, 2015

Copy link
Copy Markdown
Contributor

Ah sorry missed that. LGTM if CI is happy

jasnell commented Oct 29, 2015

Copy link
Copy Markdown
Member

LGTM

Revert "src: fix stuck debugger process"
9b02442 
This reverts commit ff877e9.

Reverted for breaking `node --debug-brk -e 0`.  It should immediately
quit but instead it hangs now.

PR-URL: nodejs#3585
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
test: add regression test for --debug-brk -e 0
810cc05 
Check that `node --debug-brk -e 0` immediately quits.

PR-URL: nodejs#3585
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
bnoordhuis closed this Oct 29, 2015
bnoordhuis deleted the revert-pr2778 branch October 29, 2015 14:59
bnoordhuis merged commit 810cc05 into nodejs:master Oct 29, 2015
bnoordhuis added a commit that referenced this pull request Oct 29, 2015
Revert "src: fix stuck debugger process"
1f6f123 
This reverts commit ff877e9.

Reverted for breaking `node --debug-brk -e 0`.  It should immediately
quit but instead it hangs now.

PR-URL: #3585
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
bnoordhuis added a commit that referenced this pull request Oct 29, 2015
test: add regression test for --debug-brk -e 0
37aec6a 
Check that `node --debug-brk -e 0` immediately quits.

PR-URL: #3585
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell mentioned this pull request Oct 29, 2015
Closed
bnoordhuis added a commit that referenced this pull request Oct 29, 2015
test: add regression test for --debug-brk -e 0
5e6f7c9 
Check that `node --debug-brk -e 0` immediately quits.

PR-URL: #3585
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

viirya commented Oct 29, 2015

Copy link
Copy Markdown
Contributor

Not sure about this. Correct me if I am wrong. But shouldn't --debug-brk break before the first statement? Why it should return immediately when running with --debug-brk=1234 -e 0.

bnoordhuis commented Oct 29, 2015

Copy link
Copy Markdown
Member Author

Maybe --debug-brk was a bad example but for example node --debug -e 0 had the same issue. If you file an issue we can look into changing --debug-brk because I think the intended behavior is for it to wait until a debug client connects.

The thing is though that ff877e9 introduced an unintentional change. If we decide to change how --debug-brk behaves in stable and LTS it should be intentional and documented. In that light I think the revert is justified.

bnoordhuis mentioned this pull request Oct 29, 2015
Closed

bnoordhuis commented Oct 29, 2015

Copy link
Copy Markdown
Member Author

For proactivity's sake: #3589

jasnell commented Oct 29, 2015

Copy link
Copy Markdown
Member

Landed in v4.x-staging in 5e6f7c9. No need to land in v4.x because the original commit hadn't landed there yet.

Fishrock123 mentioned this pull request Nov 6, 2015
Closed
bnoordhuis added a commit that referenced this pull request Nov 7, 2015
Revert "src: fix stuck debugger process"
d72bb1e 
This reverts commit ff877e9.

Reverted for breaking `node --debug-brk -e 0`.  It should immediately
quit but instead it hangs now.

PR-URL: #3585
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
bnoordhuis added a commit that referenced this pull request Nov 7, 2015
test: add regression test for --debug-brk -e 0
1e98d90 
Check that `node --debug-brk -e 0` immediately quits.

PR-URL: #3585
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 mentioned this pull request Nov 11, 2015
Merged
rvagg mentioned this pull request Dec 17, 2015
Closed
MylesBorins mentioned this pull request Jan 14, 2016
Closed
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

No reviews

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