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

test: improve async-hooks/test-callback-error #13559

Merged
merged 0 commits into from
Jun 21, 2017

Conversation

refack
Copy link
Contributor

@refack refack commented Jun 8, 2017

Ref: #13527

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Jun 8, 2017
@trevnorris
Copy link
Contributor

Will this conflict w/ #13554 ?

@refack refack added the wip Issues and PRs that are still a work in progress. label Jun 8, 2017
@refack
Copy link
Contributor Author

refack commented Jun 8, 2017

Will this conflict w/ #13554 ?

Somewhat, but we're both trying to stabilize it.

@refack
Copy link
Contributor Author

refack commented Jun 8, 2017

@Trott
Copy link
Member

Trott commented Jun 8, 2017

@refack On the stress test, you might want to include --timeout 60 or even --timeout 30 or less. If the problem is that the test will run but just take longer than 60 seconds to complete sometimes for whatever reason, the stress test will come up clean if you don't specify a timeout (I think).

@refack
Copy link
Contributor Author

refack commented Jun 8, 2017

@refack On the stress test, you might want to include --timeout 60 or even --timeout 30 or less. If the problem is that the test will run but just take longer than 60 seconds to complete sometimes for whatever reason, the stress test will come up clean if you don't specify a timeout (I think).

From my manual inspection if all's well it should complete quite quick

@refack
Copy link
Contributor Author

refack commented Jun 8, 2017

Restressing with --timeout 30 (and timestamps): https://ci.nodejs.org/job/node-stress-single-test/1282/nodes=osx1010/

@AndreasMadsen
Copy link
Member

If you are working on this then please also add the if (process.argv[2]) wrapper

if (process.argv[2]) {
  // child
} else {
  // parent with assert
}

such we don't depend on the test to work (child must throw) for it not to fork recursively.

@refack
Copy link
Contributor Author

refack commented Jun 8, 2017

If you are working on this then please also add the if (process.argv[2]) wrapper

Added returns
Also the stdout/stderr parsing was Windows incompatible

@refack refack force-pushed the less-errors-from-callback-error branch from ebd2443 to 13818d3 Compare June 9, 2017 03:07
@refack refack force-pushed the less-errors-from-callback-error branch 2 times, most recently from 9c5f6d9 to 0ad744d Compare June 9, 2017 03:56
@refack
Copy link
Contributor Author

refack commented Jun 9, 2017

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Jun 9, 2017

The only relation between --abort-on-uncaught-exception and async_hooks I can think of is src/env-inl.h#L155L159.

Comparing with process.abort() I don't know why fprintf(stderr, "\n"); fflush(stderr); is needed, that being said I also don't see any problem with it.

@Trott
Copy link
Member

Trott commented Jun 9, 2017

/cc @DavidCai1993

@refack refack force-pushed the less-errors-from-callback-error branch from a01c9d6 to e351560 Compare June 9, 2017 17:46
@refack
Copy link
Contributor Author

refack commented Jun 9, 2017

@refack refack added process Issues and PRs related to the process subsystem. and removed wip Issues and PRs that are still a work in progress. labels Jun 9, 2017
@refack
Copy link
Contributor Author

refack commented Jun 9, 2017

@refack
Copy link
Contributor Author

refack commented Jun 9, 2017

@Trott something changed. CI stopped repoducing

@refack refack force-pushed the less-errors-from-callback-error branch from 0889b3c to a5a53a4 Compare June 10, 2017 20:40
@refack
Copy link
Contributor Author

refack commented Jun 10, 2017

@refack
Copy link
Contributor Author

refack commented Jun 10, 2017

@rvagg is it possible that the outage (nodejs/build#749) fixed this?
(tl;dr process.abort() on the macOS CI machine took > 20s about two days ago now it's ~100ms)

@AndreasMadsen
Copy link
Member

Can you run the normal CI on all platforms?

CI: https://ci.nodejs.org/job/node-test-pull-request/8727/

@refack refack force-pushed the less-errors-from-callback-error branch from 4171adb to 3185544 Compare June 18, 2017 22:01
@Trott Trott dismissed their stale review June 18, 2017 22:24

requested changes done, dismissing review

@refack
Copy link
Contributor Author

refack commented Jun 18, 2017

@refack
Copy link
Contributor Author

refack commented Jun 19, 2017

So all but one machines passed SIGABRT
ubuntu1604 passed SIGSEGV

2	async-hooks/test-callback-error	
duration_ms	0.761
severity	fail
stack	|-
assert.js:60
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 'SIGSEGV' === 'SIGABRT'
    at ChildProcess.child.on (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1604_docker_alpine34-64/test/async-hooks/test-callback-error.js:98:14)
    at emitTwo (events.js:125:13)
    at ChildProcess.emit (events.js:213:7)
    at maybeClose (internal/child_process.js:898:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:209:5)

Is this a bug or an acceptable value?
@gireeshpunathil

@refack
Copy link
Contributor Author

refack commented Jun 20, 2017

Rerunning linux to see if SIGSEGV is consistent: https://ci.nodejs.org/job/node-test-commit-linux/10708/

@refack
Copy link
Contributor Author

refack commented Jun 20, 2017

So it's consistent but it's docker_alpine34 Ubuntu is just the host

@refack
Copy link
Contributor Author

refack commented Jun 20, 2017

Added bail clause if alpine acts strange.
https://ci.nodejs.org/job/node-test-commit/10691/
I'm gonna land this since test-requireio-osx1010-x64-1 is in a bad mood again.

@refack refack force-pushed the less-errors-from-callback-error branch from 7d4eac2 to 5d01063 Compare June 20, 2017 20:52
@refack
Copy link
Contributor Author

refack commented Jun 20, 2017

@refack refack closed this Jun 21, 2017
@refack refack force-pushed the less-errors-from-callback-error branch from 5d01063 to 32c7f11 Compare June 21, 2017 10:14
@refack refack merged commit 32c7f11 into nodejs:master Jun 21, 2017
@refack refack deleted the less-errors-from-callback-error branch June 21, 2017 10:16
@refack
Copy link
Contributor Author

refack commented Jun 21, 2017

@Trott
Copy link
Member

Trott commented Jun 21, 2017

Seems like the test is now failing every single time rather than intermittently. For me at least, make -j4 test on local macOS machines now always fail with this change. I think we might want to undo this until we get that sorted out? make -j4 test failing consistently means we have a broken build IMO.

@nodejs/platform-macos

@Trott
Copy link
Member

Trott commented Jun 21, 2017

Repeating what I said in another issue: Just to be clear: No shame intended! A lot of great work has happened so far and some really important information has been uncovered. These things happen.

@refack
Copy link
Contributor Author

refack commented Jun 21, 2017

Repeating what I said in another issue: Just to be clear: No shame intended! A lot of great work has happened so far and some really important information has been uncovered. These things happen.

Most of the work that gone into this PR was to allow for a more informative fails...

@refack refack restored the less-errors-from-callback-error branch June 21, 2017 18:58
@gireeshpunathil
Copy link
Member

@refack - sorry, I missed my mention in the PR. SIGSEGV seems to be unexpected, and I have seen this in AIX as well which is identified as a memory corruption. Working on some plans to track the offending code at the moment. Where should we track the Linux failure?

@refack
Copy link
Contributor Author

refack commented Jun 22, 2017

@refack - sorry, I missed my mention in the PR. SIGSEGV seems to be unexpected, and I have seen this in AIX as well which is identified as a memory corruption. Working on some plans to track the offending code at the moment. Where should we track the Linux failure?

I've opened #13865. commented that maybe that how musl libc implements abort.

addaleax pushed a commit that referenced this pull request Jun 24, 2017
PR-URL: #13559
Fixes: #13527
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
@addaleax addaleax mentioned this pull request Jun 24, 2017
addaleax pushed a commit that referenced this pull request Jun 29, 2017
PR-URL: #13559
Fixes: #13527
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
@refack refack deleted the less-errors-from-callback-error branch July 9, 2017 17:19
addaleax pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #13559
Fixes: #13527
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
PR-URL: #13559
Fixes: #13527
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
@asfourco
Copy link

asfourco commented Oct 6, 2017

ping @jasnell

@refack refack removed their assignment Oct 12, 2018
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. process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants