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

Fix cctest #14749

Closed
wants to merge 4 commits into from
Closed

Fix cctest #14749

wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

Fixes: #14206

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)

src/env, test

@addaleax
src: add overlooked handle to cleanup
5a0ee6d 
The `Environment::destroy_ids_timer_handle` should be cleaned up
by `Environment::CleanupHandles()`. Fix that by adding it to the list.

This partially fixes a cctest.

Ref: nodejs#14206
@addaleax
test: properly order freeing resources in cctest
682a951 
The `IsolateData` instance is created before the `Environment` instance,
so free in reverse order.

Fixes: nodejs#14206
@addaleax
Revert "test: add DISABLED_ prefix to commented out test"
3f95be9 
This reverts commit 75bf8a9.

Ref: nodejs#14206
Ref: nodejs#14317
@addaleax
Revert "test: disable MultipleEnvironmentsPerIsolate"
557eb7b 
This reverts commit 95ab966.

Ref: nodejs#14206
Ref: nodejs#14246
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 10, 2017
@addaleax addaleax mentioned this pull request Aug 10, 2017
Closed
@addaleax
Copy link
Member Author

I couldn’t reproduce the crash locally, but we’ve had this issue before with the same symptoms, and valgrind went from complaining to not complaining, so I’m feeling very confident that this is sufficient.

CI: https://ci.nodejs.org/job/node-test-commit/11693/

@addaleax addaleax requested a review from danbev August 10, 2017 18:59
addaleax
addaleax commented Aug 10, 2017
View reviewed changes
src/env.cc
RegisterHandleCleanup(
reinterpret_cast<uv_handle_t*>(&destroy_ids_timer_handle_),
close_and_finish,
nullptr);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris FYI, this comes from 1e44fd9. I probably would have made the same mistake, but I wanted to let you know :)

bnoordhuis
bnoordhuis approved these changes Aug 10, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

@danbev
Copy link
Contributor

danbev commented Aug 10, 2017

I couldn’t reproduce the crash locally, but we’ve had this issue before with the same symptoms, and valgrind went from complaining to not complaining, so I’m feeling very confident that this is sufficient.

I'm able to reproduce this contently on arm here and I'll run this PR and report back.

danbev
danbev approved these changes Aug 10, 2017
View reviewed changes
Copy link
Contributor

@danbev danbev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this and sorry about causing it!

refack
refack approved these changes Aug 10, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May your prefered deity bless you!

@refack
Copy link
Contributor

refack commented Aug 10, 2017

I'm +1 for fast tracking this (change is not big, and arm7 CI machine seems to consistently fail on this now)

@refack
Copy link
Contributor

refack commented Aug 10, 2017

/cc @nodejs/testing @jBarz

@refack refack added the test Issues and PRs related to the tests. label Aug 10, 2017
@addaleax
Copy link
Member Author

Let’s wait for CI to come back, but yes, I’m okay with fast-tracking as well, these aren’t so terribly complex fixes.

@addaleax
Copy link
Member Author

CI is as green as the Windows XP default background!

Landed in 97c4394...c80d400

@addaleax addaleax closed this Aug 10, 2017
@addaleax addaleax deleted the fix-cctest branch August 10, 2017 20:11
addaleax added a commit that referenced this pull request Aug 10, 2017
@addaleax
src: add overlooked handle to cleanup
b559e66 
The `Environment::destroy_ids_timer_handle` should be cleaned up
by `Environment::CleanupHandles()`. Fix that by adding it to the list.

This partially fixes a cctest.

Ref: #14206
PR-URL: #14749
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
addaleax added a commit that referenced this pull request Aug 10, 2017
@addaleax
test: properly order freeing resources in cctest
93f25f4 
The `IsolateData` instance is created before the `Environment` instance,
so free in reverse order.

Fixes: #14206
PR-URL: #14749
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
addaleax added a commit that referenced this pull request Aug 10, 2017
@addaleax
Revert "test: add DISABLED_ prefix to commented out test"
ef7da42 
This reverts commit 75bf8a9.

Ref: #14206
Ref: #14317
PR-URL: #14749
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
addaleax added a commit that referenced this pull request Aug 10, 2017
@addaleax
Revert "test: disable MultipleEnvironmentsPerIsolate"
c80d400 
This reverts commit 95ab966.

Ref: #14206
Ref: #14246
PR-URL: #14749
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
addaleax added a commit that referenced this pull request Aug 10, 2017
@addaleax
src: add overlooked handle to cleanup
033773c 
The `Environment::destroy_ids_timer_handle` should be cleaned up
by `Environment::CleanupHandles()`. Fix that by adding it to the list.

This partially fixes a cctest.

Ref: #14206
PR-URL: #14749
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
addaleax added a commit that referenced this pull request Aug 10, 2017
@addaleax
test: properly order freeing resources in cctest
0bc3124 
The `IsolateData` instance is created before the `Environment` instance,
so free in reverse order.

Fixes: #14206
PR-URL: #14749
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
addaleax added a commit that referenced this pull request Aug 10, 2017
@addaleax
Revert "test: add DISABLED_ prefix to commented out test"
8ff2a5c 
This reverts commit 75bf8a9.

Ref: #14206
Ref: #14317
PR-URL: #14749
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
addaleax added a commit that referenced this pull request Aug 10, 2017
@addaleax
Revert "test: disable MultipleEnvironmentsPerIsolate"
a781bb4 
This reverts commit 95ab966.

Ref: #14206
Ref: #14246
PR-URL: #14749
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@addaleax addaleax mentioned this pull request Aug 13, 2017
Merged
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@refack refack mentioned this pull request Sep 28, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@danbev danbev danbev approved these changes

@cjihrig cjihrig cjihrig approved these changes

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@addaleax @danbev @refack @MylesBorins @bnoordhuis @cjihrig @nodejs-github-bot