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.

Sign up for GitHub

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

Jump to bottom

test,vm: fix flaky test-util-sigint-watchdog #7933

Closed
wants to merge 2 commits into from
Closed

test,vm: fix flaky test-util-sigint-watchdog #7933

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Aug 1, 2016 •
edited
Loading

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

test,util

Description of change

@addaleax
src: use RAII for mutexes in node_watchdog.cc
5e3538b
@addaleax
test,util: fix flaky test-util-sigint-watchdog
36aa89b 
Fix parallel/test-util-sigint-watchdog by polling until the
signal has definitely been received instead of just using a timeout.

Fixes: nodejs#7919
@addaleax addaleax added util Issues and PRs related to the built-in util module. test Issues and PRs related to the tests. labels Aug 1, 2016
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 1, 2016
@addaleax
Copy link
Member Author

addaleax commented Aug 1, 2016 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-commit/4346/
FreeBSD stress test: https://ci.nodejs.org/job/node-stress-single-test/843/nodes=freebsd10-64/
stress test with master: https://ci.nodejs.org/job/node-stress-single-test/844/nodes=freebsd10-64/

@addaleax addaleax mentioned this pull request Aug 1, 2016
Closed
@cjihrig cjihrig mentioned this pull request Aug 1, 2016
Closed
2 tasks
@addaleax
Copy link
Member Author

addaleax commented Aug 1, 2016

The stress test was green, but one with more runs to be sure (the master stress test failed with just 1 failure/1000 runs): https://ci.nodejs.org/job/node-stress-single-test/845/nodes=freebsd10-64/

@Trott
Copy link
Member

Trott commented Aug 1, 2016 •
edited
Loading

One more CI: https://ci.nodejs.org/job/node-test-pull-request/3491/ (previous CI had a failed build on FreeBSD but was otherwise clean)

@Trott
Copy link
Member

Trott commented Aug 2, 2016

CI is green. Not sure who should be pinged for review on src/node_*.cc files. git shortlog suggests maybe @bnoordhuis @indutny @trevnorris @rvagg

@jasnell
Copy link
Member

jasnell commented Aug 3, 2016

LGTM but let's get @bnoordhuis to take a look also.

@Trott
Copy link
Member

Trott commented Aug 4, 2016

Bump! @bnoordhuis maybe? @cjihrig maybe? @saghul maybe?

@cjihrig
Copy link
Contributor

cjihrig commented Aug 4, 2016

@addaleax is the first commit necessary to fix the flaky test?

@saghul
Copy link
Member

saghul commented Aug 4, 2016

Sorry, I'm afraid I can't be of much help here :-S

@addaleax
Copy link
Member Author

addaleax commented Aug 4, 2016

@cjihrig No, if you think that it makes reviewing a lot easier, I can leave it out.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 4, 2016

I just wanted to make sure I was understanding correctly. I don't have a strong opinion. It does seem to simplify the code a bit, but I'll defer to @bnoordhuis there.

LGTM with or without the mutex change.

@Trott
Copy link
Member

Trott commented Aug 11, 2016

Bump.

@addaleax
Copy link
Member Author

addaleax commented Aug 11, 2016 •
edited
Loading

landed in e48b01a, thanks for the bump.

@addaleax addaleax closed this Aug 11, 2016
@addaleax addaleax deleted the fix-test-util-sigint-watchdog branch August 11, 2016 16:38
@cjihrig
Copy link
Contributor

cjihrig commented Aug 11, 2016

Looks like the commit is missing a few pieces of metadata.

@addaleax
Copy link
Member Author

Dang, forgot that again… fixed in a moment

addaleax added a commit that referenced this pull request Aug 11, 2016
@addaleax
src: use RAII for mutexes in node_watchdog.cc
84f0778 
PR-URL: #7933
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax added a commit that referenced this pull request Aug 11, 2016
@addaleax
test,util: fix flaky test-util-sigint-watchdog
6d3241d 
Fix parallel/test-util-sigint-watchdog by polling until the
signal has definitely been received instead of just using a timeout.

Fixes: #7919
PR-URL: #7933
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax
Copy link
Member Author

Thanks for noticing, landed in 84f0778, 6d3241d.

cjihrig pushed a commit that referenced this pull request Aug 11, 2016
@addaleax @cjihrig
src: use RAII for mutexes in node_watchdog.cc
fb8840c 
PR-URL: #7933
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
cjihrig pushed a commit that referenced this pull request Aug 11, 2016
@addaleax @cjihrig
test,util: fix flaky test-util-sigint-watchdog
4416ffa 
Fix parallel/test-util-sigint-watchdog by polling until the
signal has definitely been received instead of just using a timeout.

Fixes: #7919
PR-URL: #7933
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
Merged
@MylesBorins
Copy link
Contributor

@addaleax should this be backported?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. test Issues and PRs related to the tests. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate flaky test-util-sigint-watchdog on FreeBSD
7 participants
@addaleax @Trott @jasnell @cjihrig @saghul @MylesBorins @nodejs-github-bot