Skip to content

debugger: fix debugger blocked main thread - #5270

Closed
yjhjstz wants to merge 1 commit into
nodejs:masterfrom
yjhjstz:fix-debugger-block
Closed

yjhjstz wants to merge 1 commit into
nodejs:masterfrom
yjhjstz:fix-debugger-block

Conversation

@yjhjstz

@yjhjstz yjhjstz commented Feb 17, 2016

Copy link
Copy Markdown

see #2110
Call uv_walk to close all handles, otherwise would meat error below.

Assertion failed: ((err) == (0)), function Stop, file ../src/debug-agent.cc, line 156.

@jasnell

jasnell commented Feb 17, 2016

Copy link
Copy Markdown
Member

@bnoordhuis

Copy link
Copy Markdown
Member

I don't think blindly closing all handles is the best way to fix this (although it's certainly the easiest) because it introduces memory leaks and corrupts the HandleWrap linked list. #2133 has a solution for cleaning up handles at exit.

@yjhjstz

yjhjstz commented Feb 23, 2016

Copy link
Copy Markdown
Author

yes, in fact I just want to close tty handle. but will #2133 to be merge?

@bnoordhuis

Copy link
Copy Markdown
Member

Perhaps we can land the cleanup changes. I don't really have time to review it, though.

@jasnell

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

@thealphanerd ... any thoughts on this one?

@MylesBorins MylesBorins self-assigned this Mar 24, 2016
@MylesBorins

Copy link
Copy Markdown
Contributor

Nothing off the top of my head. I've assigned it to myself and will investigate

@MylesBorins MylesBorins assigned indutny and unassigned MylesBorins Apr 8, 2016
@MylesBorins

Copy link
Copy Markdown
Contributor

@indutny did you have a chance to check this out, it seems like you gave the origin review in #2110

@MylesBorins

Copy link
Copy Markdown
Contributor

the test is failing for me locally on osx

@jjqq2013

Copy link
Copy Markdown
Contributor

+1

@rvagg
rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01
@MylesBorins

Copy link
Copy Markdown
Contributor

Is this something we want to try and fix in LTS?

@jasnell

jasnell commented Dec 29, 2016

Copy link
Copy Markdown
Member

Possibly. If there is no negative impact then I don't see why not.

@jasnell jasnell added the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Feb 28, 2017
@jasnell

jasnell commented Mar 1, 2017

Copy link
Copy Markdown
Member

@MylesBorins ... is this still needed? given that the debugger is deprecated and going away, I'm inclined to close

@MylesBorins

Copy link
Copy Markdown
Contributor

@jasnell I'm open to closing it... that being said if it can fix 4.x it may be worth it. No one is stepping up to do it though...

@jjqq2013

jjqq2013 commented Mar 1, 2017 •

Copy link
Copy Markdown
Contributor

This is a confusing problem, causing debugger not automatically exited. We should fix it

@fhinkel

fhinkel commented Mar 26, 2017

Copy link
Copy Markdown
Contributor

Feel free to reopen this PR if you get back to working on it.

@fhinkel fhinkel closed this Mar 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stalled Issues and PRs manually marked as stalled and scheduled for automatic closure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants