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

investigate flaky inspector/test-inspector on Windows #8804

Closed
Trott opened this issue Sep 27, 2016 · 9 comments
Closed

investigate flaky inspector/test-inspector on Windows #8804

Trott opened this issue Sep 27, 2016 · 9 comments
Labels
inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.

Comments

@Trott
Copy link
Member

Trott commented Sep 27, 2016

Fails intermittently on Windows 2012r2 / VS 2015 on CI.

https://ci.nodejs.org/job/node-test-binary-windows/4057/RUN_SUBSET=1,VS_VERSION=vs2015,label=win2012r2/console:

not ok 296 inspector/test-inspector
# Error: read ECONNRESET
#     at exports._errnoException (util.js:1026:11)
#     at TCP.onread (net.js:572:26)
# [err] Debugger listening on port 9229.
# [err] Warning: This is an experimental feature and could change at any time.
# [err] To start debugging, open the following URL in Chrome:
# [err]     chrome-devtools://devtools/remote/serve_file/@60cd6e859b9f557d2312f5bf532f6aec5f284980/inspector.html?experiments=true&v8only=true&ws=localhost:9229/node
# [err] 
# [test] Verifying debugger stops on start (--debug-brk option)
# [test] Setting a breakpoint and verifying it is hit
# [out] A message 5
# [out] 
# [test] Verify we can read current application state
# [test] Verify node waits for the frontend to disconnect
# [out] Outputed message #1
# [out] 
# [err] Debugger attached.
# [err] Waiting for the debugger to disconnect...
# [err] 
# [test] Connection terminated
  ---
  duration_ms: 0.536

/cc @eugeneo

@Trott Trott added windows Issues and PRs related to the Windows platform. test Issues and PRs related to the tests. inspector Issues and PRs related to the V8 inspector protocol labels Sep 27, 2016
@mcollina
Copy link
Member

This might be related: #8155.

@Trott
Copy link
Member Author

Trott commented Nov 16, 2016

Alas, #8155 is fixed but we're still seeing this.

Any progress with https://github.com/eugeneo/node/commit/3d3ec6fd960b52e9ae920c2c7b387c0c37bf398d? (Or did that already land in some format?)

Here's a failure from today which has error output that looks slightly different than previous failures. Not sure if Jenkins changed or if the inspector code did or what....

https://ci.nodejs.org/job/node-test-binary-windows/4783/RUN_SUBSET=2,VS_VERSION=vs2015,label=win2008r2/console

not ok 302 inspector/test-inspector # TODO : Fix flaky test
  ---
  duration_ms: 0.522
  severity: flaky
  stack: |-
    [err] Debugger listening on port 9229.
    [err] Warning: This is an experimental feature and could change at any time.
    [err] To start debugging, open the following URL in Chrome:
    [err]     chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=127.0.0.1:9229/63a05fcc-03d6-43f3-bf70-550e1e4101ce
    [err] 
    [test] Verifying debugger stops on start (--debug-brk option)
    [test] Setting a breakpoint and verifying it is hit
    [out] A message 5
    [out] 
    [test] Verify we can read current application state
    [test] Verify sending and receiving UTF8 characters
    [out] טֶ字и
    [out] 
    [test] Verify node waits for the frontend to disconnect
    [out] Outputed message #1
    [out] 
    [err] Debugger attached.
    [err] Waiting for the debugger to disconnect...
    [err] 
    [test] Connection terminated
    Error: read ECONNRESET
        at exports._errnoException (util.js:1022:11)
        at TCP.onread (net.js:572:26)
  ...

@eugeneo
Copy link
Contributor

eugeneo commented Nov 16, 2016

I am trying some fixes from time to time, no actual progress yet...

@Trott
Copy link
Member Author

Trott commented Nov 17, 2016

The "ECONNRESET showing up only on Windows" has certainly happened elsewhere, such as #5386. /cc @gibfahn in case they have any light to shed

Maybe someone in @nodejs/platform-windows knows something about why that is and what the appropriate way of dealing with it is. (Swallow ECONNRESET on Windows only?)

@nodejs/testing

@gibfahn
Copy link
Member

gibfahn commented Nov 17, 2016

@Trott I think the problem is that no-one ever got to the bottom of why the ECONNRESET actually happens. Unless we know why it's happening, I don't think we should just ignore what could well be a legitimate error.

Maybe if we could work out the simplest reproducible test case, we could have a test that specifically checks for this, mark that one as flaky, and then swallow ECONNRESET on all the other tests (e.g. this one).

@Trott
Copy link
Member Author

Trott commented Nov 18, 2016

@Trott I think the problem is that no-one ever got to the bottom of why the ECONNRESET actually happens.

Hmmm...someone who cares about tests and understands Node.js tests...and who understands Windows and Node.js Windows quirks... Maybe @joaocgreis would have some ideas as to how we might trigger the ECONNRESET-on-Windows issue reliably?

@Trott
Copy link
Member Author

Trott commented Nov 20, 2016

I ran the stress test 100 times in succession and it failed 93 times. Yikes! https://ci.nodejs.org/job/node-stress-single-test/1063/nodes=win10/console

@Trott
Copy link
Member Author

Trott commented Nov 21, 2016

Changing socket.end() to socket.destroy() seems to fix the flakiness on Windows:

https://ci.nodejs.org/job/node-stress-single-test/1066/nodes=win10/console

Any reason not to go with something like that?

@Trott
Copy link
Member Author

Trott commented Nov 21, 2016

PR to fix (if the destroy() solution is OK): #9727

Trott added a commit to Trott/io.js that referenced this issue Nov 22, 2016
Using `socket.destroy()` instead of `socket.end()` fixes
more-than-intermittent ECONNRESET issues on Windows.

Fixes: nodejs#8804
@Trott Trott closed this as completed in 2486273 Nov 23, 2016
addaleax pushed a commit that referenced this issue Dec 5, 2016
Using `socket.destroy()` instead of `socket.end()` fixes
more-than-intermittent ECONNRESET issues on Windows.

PR-URL: #9727
Fixes: #8804
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
addaleax pushed a commit to addaleax/node that referenced this issue Dec 8, 2016
Using `socket.destroy()` instead of `socket.end()` fixes
more-than-intermittent ECONNRESET issues on Windows.

PR-URL: nodejs#9727
Fixes: nodejs#8804
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
MylesBorins pushed a commit that referenced this issue Dec 21, 2016
Using `socket.destroy()` instead of `socket.end()` fixes
more-than-intermittent ECONNRESET issues on Windows.

PR-URL: #9727
Fixes: #8804
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants