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.

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

Revert "src: fix stuck debugger process" #3585

Merged
merged 2 commits into from Oct 29, 2015

Conversation

Projects
None yet
6 participants
@bnoordhuis
Copy link
Member

commented Oct 29, 2015

@evanlucas

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

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

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2015

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

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

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

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

Ah sorry missed that. LGTM if CI is happy

@jasnell

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

LGTM

bnoordhuis added some commits Oct 29, 2015

Revert "src: fix stuck debugger process"
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>
test: add regression test for --debug-brk -e 0
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>

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:revert-pr2778 branch to 810cc05 Oct 29, 2015

@bnoordhuis bnoordhuis closed this Oct 29, 2015

@bnoordhuis bnoordhuis deleted the bnoordhuis:revert-pr2778 branch Oct 29, 2015

@bnoordhuis 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"
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
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>

bnoordhuis added a commit that referenced this pull request Oct 29, 2015

test: add regression test for --debug-brk -e 0
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

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2015

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

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2015

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

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2015

For proactivity's sake: #3589

@jasnell

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

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

@jasnell jasnell removed the lts-watch-v4.x label Oct 29, 2015

bnoordhuis added a commit that referenced this pull request Nov 7, 2015

Revert "src: fix stuck debugger process"
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
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 Fishrock123 referenced this pull request Nov 11, 2015

Merged

Propose v5.1.0 #3736

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.