FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: changed the buffer offset by thangktran · Pull Request #31171 · nodejs/node · GitHub

/ node Public

test: changed the buffer offset - #31171

Closed
thangktran wants to merge 1 commit into
nodejs:masterfrom
thangktran:thangktran/issues/31061
ConversationCommits (1)ChecksFiles changed
Closed

test: changed the buffer offset#31171
thangktran wants to merge 1 commit into
nodejs:masterfrom
thangktran:thangktran/issues/31061

Conversation

thangktran commented Jan 3, 2020

Copy link
Copy Markdown
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 added addons Issues and PRs related to native addons. test Issues and PRs related to the tests. labels Jan 3, 2020

thangktran commented Jan 3, 2020

Copy link
Copy Markdown
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 commented Jan 3, 2020

Copy link
Copy Markdown
Member

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

richardlau reviewed Jan 3, 2020
View reviewed changes
Comment thread test/addons/buffer-free-callback/test.js Outdated
richardlau approved these changes Jan 3, 2020
View reviewed changes

richardlau left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 3, 2020

nodejs-github-bot commented Jan 3, 2020

Copy link
Copy Markdown
Collaborator

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

thangktran force-pushed the thangktran/issues/31061 branch from 3261259 to aa74951 Compare January 3, 2020 16:41

thangktran commented Jan 3, 2020

Copy link
Copy Markdown
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

richardlau reviewed Jan 3, 2020
View reviewed changes
Comment thread test/addons/buffer-free-callback/test.js Outdated
thangktran force-pushed the thangktran/issues/31061 branch from aa74951 to 8494f9b Compare January 4, 2020 01:21
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
thangktran force-pushed the thangktran/issues/31061 branch from 8494f9b to ddc6ea5 Compare January 4, 2020 08:00

nodejs-github-bot commented Jan 5, 2020

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jan 5, 2020

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jan 5, 2020

Copy link
Copy Markdown
Collaborator

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

Trott commented Jan 6, 2020

Copy link
Copy Markdown
Member

Landed in 783f8c6

Trott closed this Jan 6, 2020
Trott pushed a commit that referenced this pull request Jan 6, 2020
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 <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jan 6, 2020
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 <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

sam-github commented Jan 6, 2020

Copy link
Copy Markdown
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 commented Jan 6, 2020

Copy link
Copy Markdown
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 mentioned this pull request Jan 7, 2020
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
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 <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
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 <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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 <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins mentioned this pull request Feb 8, 2020
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
richardlau richardlau approved these changes
addaleax addaleax approved these changes
+1 more reviewer
devnexen devnexen approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

addons Issues and PRs related to native addons. author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL