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 parallel/test-async-hooks-http-parser-destroy on OS X #28112

Closed
sam-github opened this issue Jun 6, 2019 · 9 comments
Closed
Labels
async_hooks Issues and PRs related to the async hooks subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. http Issues or PRs related to the http subsystem. macos Issues and PRs related to the macOS platform / OSX. test Issues and PRs related to the tests.

Comments

@sam-github
Copy link
Contributor

Seems to be failing regularly:

For all above, failures are identical:

timeout
assert.js:89
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected ... Lines skipped

  [
-   1288,
    156,

Perhaps it should be marked flaky? I'm not sure how common it has to be to deserve the marking.

  • Version:
  • Platform:
  • Subsystem:
@sam-github sam-github added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. macos Issues and PRs related to the macOS platform / OSX. async_hooks Issues and PRs related to the async hooks subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. labels Jun 6, 2019
@Flarna
Copy link
Member

Flarna commented Jun 13, 2019

The test duration in failed cases seems to be significant longer as in the pass case. Not sure if duration_ms: 120.95 is really in milli seconds - as a wild guess I would say it's in sec and therefore 2 minutes which would match the default http timeout used by node as far as I know.

I have no access to OS X so hard for me to work on this.

@Trott
Copy link
Member

Trott commented Jun 13, 2019

The test duration in failed cases seems to be significant longer as in the pass case. Not sure if duration_ms: 120.95 is really in milli seconds - as a wild guess I would say it's in sec and therefore 2 minutes which would match the default http timeout used by node as far as I know.

I have no access to OS X so hard for me to work on this.

duraion_ms is unfortunate, but it's a tap thing so we can't/shouldn't change it. It is duration in seconds.

It fails at 120 seconds because that's the timeout duration in the test runner.

So it gets caught in a deadlock or infinite loop or waiting for an event that never actually fires or (unlikely but possible) maybe just takes too darned long to run and would complete if it had more time or...

@Flarna
Copy link
Member

Flarna commented Jun 13, 2019

Hmm, maybe add some more counters, e.g. one for HTTPINCOMINGMESSAGE and HTTPCLIENTREQUEST, one for client responses and then assert on them first before the diff of the list to get some hint what is hanging.

Or hope that some OS X users gives a try to reproduce this locally and debug.

@sam-github
Copy link
Contributor Author

@Flarna Are you willing to PR that change, ☝️ ? It would be very helpful if you have time to start looking into this.

@Flarna
Copy link
Member

Flarna commented Jun 13, 2019

Sure, will take a look in a free slot during next days.

created #28253

@Flarna
Copy link
Member

Flarna commented Jun 28, 2019

Is this still valid? PR is open since quite a while now...

Flarna added a commit to dynatrace-oss-contrib/node that referenced this issue Jul 11, 2019
Improve asserts to distinguish between reequest and response parsers.

Change the assert sequence to first assert on the number of ids to
easier identify if some operation is missing/incomplete.

Destroy HTTP agent once expected number of events have been seen to
avoid waiting on socket timeouts.

Refs: nodejs#28112
@lpinca
Copy link
Member

lpinca commented Jul 13, 2019

I can't reproduce locally but I'm on macOS 10.14 not 10.11.

Trott pushed a commit to Trott/io.js that referenced this issue Jul 30, 2019
Improve asserts to distinguish between reequest and response parsers.

Change the assert sequence to first assert on the number of ids to
easier identify if some operation is missing/incomplete.

Destroy HTTP agent once expected number of events have been seen to
avoid waiting on socket timeouts.

Refs: nodejs#28112

PR-URL: nodejs#28253
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this issue Aug 2, 2019
Improve asserts to distinguish between reequest and response parsers.

Change the assert sequence to first assert on the number of ids to
easier identify if some operation is missing/incomplete.

Destroy HTTP agent once expected number of events have been seen to
avoid waiting on socket timeouts.

Refs: #28112

PR-URL: #28253
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@Flarna
Copy link
Member

Flarna commented Sep 14, 2019

Has #28253 fixed the flaky parallel/test-async-hooks-http-parser-destroy on OS X? Don't know how to query CI statistics.

@Trott
Copy link
Member

Trott commented Sep 15, 2019

Has #28253 fixed the flaky parallel/test-async-hooks-http-parser-destroy on OS X? Don't know how to query CI statistics.

Used ncu-ci from node-core-utils to check and couldn't find any async-hooks test failures other than test-statwatcher.js. So I think we can conclude that this isn't a problem and plan to re-open if that's wrong and someone notices it poppingup again.

Thanks, @Flarna!

@Trott Trott closed this as completed Sep 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. http Issues or PRs related to the http subsystem. macos Issues and PRs related to the macOS platform / OSX. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

4 participants