debugger: fix debugger blocked main thread #2151
Conversation
|
|
||
| var nodeProcess; | ||
|
|
||
| function doTest() { |
There was a problem hiding this comment.
You don't need this function and run as well. Write the contents of the function in the top level itself.
|
LGTM, except one nit (see @thefourtheye's comment). Thanks! |
|
Also, the commit message should preferrably include your actual name and a valid email address, so that the commits may be attributed to your name (github account). Please refer https://help.github.com/articles/setting-your-email-in-git/#setting-your-local-git-email-address-using-the-git-config-command and https://help.github.com/articles/setting-your-email-in-git/#commits-on-github-arent-linking-to-my-account |
|
Looks like the linter is not happy about it too. @yjhjstz have you tried running @thefourtheye good point, thanks! |
|
make test & make jshint |
|
CI looks green (except unrelated changes). |
|
@yjhjstz may I ask you to include your name in the commit? You may do it this way: Other than that - no comments, looks awesome. Please let me know when I can re-review it ;) |
|
You mean I should modify "yjhjstz" to "Jianghua Yang" , right? |
|
This is correct! |
|
Great! There is one small problem, though. The test hangs, and sometimes crashes: It seems that the debuggers tests are not being ran by default. May I ask you to give it a try, and probably figure out what's going on? :) P.S. Debugger tests could be run with |
|
Cool, what about the test hanging? ;) It doesn't look like this patch affects the way this test works. |
|
Any error msg? I always encountered
if main thread blocked, it would be killed by child process in 5 secs. |
|
@yjhjstz it just hangs on my computer, whether with your patch, or without it. |
|
Yes, in mac os, It will block about 5 secs, that's because iojs can't handle |
|
@yjhjstz sorry for delay. The idea of the test is that it should pass everywhere with the patch, and should fail or crash without it. Is there any way to reliably test it? |
| @@ -0,0 +1,28 @@ | |||
| 'use strict'; | |||
| var common = require('../common'); | |||
There was a problem hiding this comment.
Use const wherever the values are assigned once and never reassigned.
|
This will definitely leak some resources. What are the handles that are alive at this time? Let's figure out how to close them properly. Thanks for the PR, though! |
|
add close handles , please step to PR #2622 |
see #2110 include a testcase.
when call
process._debugEnd()before fixed , it would be blocked, then throw a timeout error.@thefourtheye @indutny