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: changed the buffer offset #31171

Closed
wants to merge 1 commit into from
Closed

test: changed the buffer offset #31171

wants to merge 1 commit into from

Conversation

thangktran
Copy link
Contributor

to avoid problem with the new behaviour of new V8 BackingStore API. By
changing the offset, the base address of each test case will be
different.

Fixes: #31061

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. test Issues and PRs related to the tests. labels Jan 3, 2020
@thangktran
Copy link
Contributor Author

/cc @addaleax @richardlau

Could someone please run a stress test on this PR to verify that the flake is fixed. Thank you.

@addaleax
Copy link
Member

addaleax commented Jan 3, 2020

Stress test CI: https://ci.nodejs.org/job/node-stress-single-test/35/

richardlau
richardlau approved these changes Jan 3, 2020
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

Since there’s no protection in the binding I’d suggest some sort of comment in the test to discourage new check()s being added in the future that result in duplicate base addresses.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 3, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@thangktran
Copy link
Contributor Author

Since there’s no protection in the binding I’d suggest some sort of comment in the test to discourage new check()s being added in the future that result in duplicate base addresses.

fixed

@thangktran
test: changed the buffer offset
ddc6ea5 
to avoid problem with the new behaviour of new V8 BackingStore API. By
changing the offset, the base address of each test case will be
different.

Fixes: nodejs#31061
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Jan 6, 2020

Landed in 783f8c6

@Trott Trott closed this Jan 6, 2020
Trott pushed a commit that referenced this pull request Jan 6, 2020
@thangktran @Trott
test: change buffer offset to accommodate V8 BackingStore
783f8c6 
To avoid problem with the behavior of new V8 BackingStore API,
change the offset. The base address of each test case will be
different.

Fixes: #31061

PR-URL: #31171
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jan 6, 2020
@thangktran @targos
test: change buffer offset to accommodate V8 BackingStore
21d9664 
To avoid problem with the behavior of new V8 BackingStore API,
change the offset. The base address of each test case will be
different.

Fixes: #31061

PR-URL: #31171
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@sam-github
Copy link
Contributor

Is https://ci.nodejs.org/job/node-test-commit-linux-containered/17217/ / https://ci.nodejs.org/job/node-test-commit-linux-containered/17217/nodes=ubuntu1804_sharedlibs_openssl111_x64/ also a (non-AIX) symptom of the same underlying problem?

00:09:40 not ok 2531 addons/buffer-free-callback/test
00:09:40   ---
00:09:40   duration_ms: 0.426
00:09:40   severity: crashed
00:09:40   exitcode: -4
00:09:40   stack: |-
00:09:40     
00:09:40     
00:09:40     #
00:09:40     # Fatal error in , line 0
00:09:40     # Check failed: result.second.
00:09:40     #
00:09:40     #
00:09:40     #
00:09:40     #FailureMessage Object: 0x7ffd3790cd20
00:09:40      1: 0x55cde33b5151  [out/Release/node]
00:09:40      2: 0x55cde41cabce V8_Fatal(char const*, ...) [out/Release/node]
00:09:40      3: 0x55cde37e679d v8::internal::GlobalBackingStoreRegistry::Register(std::shared_ptr<v8::internal::BackingStore>) [out/Release/node]
00:09:40      4: 0x55cde3505c9e v8::ArrayBuffer::GetBackingStore() [out/Release/node]
00:09:40      5: 0x55cde3320e7f node::Buffer::New(node::Environment*, char*, unsigned long, void (*)(char*, void*), void*) [out/Release/node]
00:09:40      6: 0x55cde332151c node::Buffer::New(v8::Isolate*, char*, unsigned long, void (*)(char*, void*), void*) [out/Release/node]
00:09:40      7: 0x7f13e05a7f89 Alloc(v8::FunctionCallbackInfo<v8::Value> const&) [/home/iojs/build/workspace/node-test-commit-linux-containered/test/addons/buffer-free-callback/build/Release/binding.node]
00:09:40      8: 0x55cde354b4b3  [out/Release/node]
00:09:40      9: 0x55cde354d3af v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [out/Release/node]
00:09:40     10: 0x55cde3da43f9  [out/Release/node]
00:09:40   ...

The daily build failure predates this PR, so maybe is fixed.

@richardlau
Copy link
Member

@sam-github I believe it is -- it's failing the same check and the underlying issue that was fixed is not AIX specific.

@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
@thangktran @targos
test: change buffer offset to accommodate V8 BackingStore
961c742 
To avoid problem with the behavior of new V8 BackingStore API,
change the offset. The base address of each test case will be
different.

Fixes: #31061

PR-URL: #31171
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
@thangktran @targos
test: change buffer offset to accommodate V8 BackingStore
18cd002 
To avoid problem with the behavior of new V8 BackingStore API,
change the offset. The base address of each test case will be
different.

Fixes: #31061

PR-URL: #31171
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@thangktran @BethGriggs
test: change buffer offset to accommodate V8 BackingStore
6e17ea4 
To avoid problem with the behavior of new V8 BackingStore API,
change the offset. The base address of each test case will be
different.

Fixes: #31061

PR-URL: #31171
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@Trott Trott Trott approved these changes

@devnexen devnexen devnexen approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
addons Issues and PRs related to native addons. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

investigate flaky buffer-free-callback addon test on AIX in CI
8 participants
@thangktran @addaleax @nodejs-github-bot @Trott @sam-github @richardlau @devnexen @BridgeAR