-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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 /parallel/test-fs-stat-bigint fails 3 out of 100 times #24593
Comments
See also: #24565 |
/ping @joyeecheung |
Possible fix/clarify - #23821 |
I haven't been able to reproduce but looking at the test and the output OP posted, I suspect there's something updating the atime between the two stat calls. The tests in test-fs-stat-bigint.js follow this pattern:
There's a time window between 1 and 2 where an external actor can update the atime. The three failures all have bigint atime < non-bigint atime, probably not a coincidence. |
|
Considering some of the tests there are synchronous and some of them are asynchronous, and it fails with |
I don't think that's it. There's no sharing taking place, the typed array is passed from C++ to JS and immediately converted to a FWIW, I can reproduce when I run |
Another failure on SmartOS, seems off-by-one as in @bnoordhuis's example posted above. Is that a bug in the result or should the test actually permit off-by-one in the timestamp? (Seems like a bug but maybe there's a subtlety I'm missing. I haven't looked closely at all.) https://ci.nodejs.org/job/node-test-commit-smartos/22861/nodes=smartos16-64/console test-joyent-smartos16-x64-1 00:12:30 not ok 691 parallel/test-fs-stat-bigint
00:12:30 ---
00:12:30 duration_ms: 0.535
00:12:30 severity: fail
00:12:30 exitcode: 1
00:12:30 stack: |-
00:12:30 (node:636936) ExperimentalWarning: The fs.promises API is experimental
00:12:30 assert.js:86
00:12:30 throw new AssertionError(obj);
00:12:30 ^
00:12:30
00:12:30 AssertionError [ERR_ASSERTION]: 1546243950493n !== 1546243950494n
00:12:30 key=atimeMs, val=1546243950494
00:12:30 at verifyStats (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos16-64/test/parallel/test-fs-stat-bigint.js:66:14)
00:12:30 at fs.stat (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos16-64/test/parallel/test-fs-stat-bigint.js:109:7)
00:12:30 at FSReqCallback.oncomplete (fs.js:160:5)
00:12:30 ... @nodejs/platform-smartos @nodejs/fs |
Failure on debian9-64: https://ci.nodejs.org/job/node-test-commit-linux/24665/nodes=debian9-64/console test-softlayer-debian9-x64-1 00:05:02 not ok 593 parallel/test-fs-stat-bigint
00:05:02 ---
00:05:02 duration_ms: 0.141
00:05:02 severity: fail
00:05:02 exitcode: 1
00:05:02 stack: |-
00:05:02 assert.js:86
00:05:02 throw new AssertionError(obj);
00:05:02 ^
00:05:02
00:05:02 AssertionError [ERR_ASSERTION]: 1547539502126n !== 1547539502127n
00:05:02 key=atimeMs, val=1547539502127
00:05:02 at verifyStats (/home/iojs/build/workspace/node-test-commit-linux/nodes/debian9-64/test/parallel/test-fs-stat-bigint.js:66:14)
00:05:02 at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/debian9-64/test/parallel/test-fs-stat-bigint.js:93:3)
00:05:02 at Module._compile (internal/modules/cjs/loader.js:722:30)
00:05:02 at Object.Module._extensions..js (internal/modules/cjs/loader.js:733:10)
00:05:02 at Module.load (internal/modules/cjs/loader.js:621:32)
00:05:02 at tryModuleLoad (internal/modules/cjs/loader.js:564:12)
00:05:02 at Function.Module._load (internal/modules/cjs/loader.js:556:3)
00:05:02 at Function.Module.runMain (internal/modules/cjs/loader.js:775:12)
00:05:02 at executeUserCode (internal/bootstrap/node.js:433:15)
00:05:02 at startExecution (internal/bootstrap/node.js:370:3)
00:05:02 ... |
OS - Windows
This is the test code: node/test/parallel/test-fs-stat-bigint.js Lines 136 to 141 in 5e1d446
... node/test/parallel/test-fs-stat-bigint.js Line 22 in 5e1d446
... node/test/parallel/test-fs-stat-bigint.js Lines 65 to 70 in 5e1d446
So Is the "atime is changed between the calls" hypothesis is correct, flipping L138 and L139 should flip the sides on the inequality. I'm testing that now. |
AssertionError [ERR_ASSERTION]: 1548858177822n !== 1548858177823n
key=atimeMs, val=1548858177823
at verifyStats (D:\code\node\test\parallel\test-fs-stat-bigint.js:66:14)
at D:\code\node\test\parallel\test-fs-stat-bigint.js:140:3
Command: D:\code\node\Release\node.exe D:\code\node\test\parallel\test-fs-stat-bigint.js
[00:21|% 100|+ 99|- 1]: Done
|
@refack That implies the bug lies somewhere in the numeric conversions? |
So it seems. This might arise from the way we convert nano-seconds to mili-seconds Line 175 in 5e1d446
For BigInt we use integer aritmetic that truncates the reminder:Line 187 in 5e1d446
Is it possible that when the reminder is higher then some threshold, V8 turns it into the next "SafeInteger" node/test/parallel/test-fs-stat-bigint.js Lines 65 to 70 in 5e1d446
so that 1548858177822.9999d => Number(1548858177823) ?
P.S. quick REPL play seems like that is so: > 1548858177822.9999
1548858177823 |
Floating point rounding error was my hunch too when I first looked at this bug report and it's probably a culprit but perhaps not the only one.
That makes sense because Rounding also wouldn't explain the ~7 ms gap between |
test-joyent-ubuntu1604_sharedlibs_container-x64-5
|
https://ci.nodejs.org/job/node-test-commit-smartos/24730/nodes=smartos17-64/console test-joyent-smartos17-x64-2 15:10:28 not ok 708 parallel/test-fs-stat-bigint
15:10:28 ---
15:10:28 duration_ms: 0.425
15:10:28 severity: fail
15:10:28 exitcode: 1
15:10:28 stack: |-
15:10:28 assert.js:87
15:10:28 throw new AssertionError(obj);
15:10:28 ^
15:10:28
15:10:28 AssertionError [ERR_ASSERTION]: 1553292628257n !== 1553292628258n
15:10:28 key=mtimeMs, val=1553292628258
15:10:28 at verifyStats (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos17-64/test/parallel/test-fs-stat-bigint.js:63:14)
15:10:28 at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos17-64/test/parallel/test-fs-stat-bigint.js:98:3)
15:10:28 at Module._compile (internal/modules/cjs/loader.js:814:30)
15:10:28 at Object.Module._extensions..js (internal/modules/cjs/loader.js:825:10)
15:10:28 at Module.load (internal/modules/cjs/loader.js:681:32)
15:10:28 at tryModuleLoad (internal/modules/cjs/loader.js:613:12)
15:10:28 at Function.Module._load (internal/modules/cjs/loader.js:605:3)
15:10:28 at Function.Module.runMain (internal/modules/cjs/loader.js:877:12)
15:10:28 at internal/main/run_main_module.js:21:11
15:10:28 ... |
I have a WIP that refactors the Stats mechanism to simplify it quite a bit, and solve this on the way. Need to find some time to finish it... |
test-softlayer-ubuntu1604_sharedlibs_container-x64-5 00:07:18 not ok 735 parallel/test-fs-stat-bigint
00:07:18 ---
00:07:18 duration_ms: 0.431
00:07:18 severity: fail
00:07:18 exitcode: 1
00:07:18 stack: |-
00:07:18 /home/iojs/build/workspace/node-test-commit-linux-containered/test/common/index.js:681
00:07:18 const crashOnUnhandledRejection = (err) => { throw err; };
00:07:18 ^
00:07:18
00:07:18 AssertionError [ERR_ASSERTION]: 1554534437999n !== 1554534438000n
00:07:18 key=atimeMs, val=1554534438000
00:07:18 at verifyStats (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-fs-stat-bigint.js:63:14)
00:07:18 at /home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-fs-stat-bigint.js:147:5
00:07:18 ... |
No surprise, but still happening, just so no one gets tempted to close this quite yet: https://ci.nodejs.org/job/node-test-binary-arm-12+/945/RUN_SUBSET=3,label=pi2-docker/console 10:06:58 not ok 178 parallel/test-fs-stat-bigint
10:06:59 ---
10:06:59 duration_ms: 2.958
10:06:59 severity: fail
10:06:59 exitcode: 1
10:06:59 stack: |-
10:06:59 assert.js:89
10:06:59 throw new AssertionError(obj);
10:06:59 ^
10:06:59
10:06:59 AssertionError [ERR_ASSERTION]: 1560186418390n !== 1560186418391n
10:06:59 key=mtimeMs, val=1560186418391
10:06:59 at verifyStats (/home/iojs/build/workspace/node-test-binary-arm/test/parallel/test-fs-stat-bigint.js:63:14)
10:06:59 at /home/iojs/build/workspace/node-test-binary-arm/test/parallel/test-fs-stat-bigint.js:106:7
10:06:59 at FSReqCallback.oncomplete (fs.js:166:5) {
10:06:59 generatedMessage: false,
10:06:59 code: 'ERR_ASSERTION',
10:06:59 actual: 1560186418390n,
10:06:59 expected: 1560186418391n,
10:06:59 operator: 'strictEqual'
10:06:59 }
10:06:59 ... |
I am wondering whether #21387 may help - this moves the timespec-to-ms calculation into JS land entirely. It may make a difference for the precision loss. |
Refs: nodejs#24593 PR-URL: nodejs#28156 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Stress test against master: https://ci.nodejs.org/job/node-stress-single-test/2225/ Stress test against #21387: https://ci.nodejs.org/job/node-stress-single-test/2226/ Might have to do this a few times to figure out how to trigger it in a stress test, but maybe not. |
Refs: #24593 PR-URL: #28156 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]>
This test doesn't seem to be failing anymore. (I used ncu-ci walk to look for failures in CI and also ran the test a few thousand times locally with no failures.) I'm going to close this, but feel free to re-open if this is observed again and/or if a reliable reproduction is found. |
Stress test on FreeBSD shows this is still A Thing. |
One failure out of 1000 runs. https://ci.nodejs.org/view/Stress/job/node-stress-single-test/nodes=freebsd11-x64/41/console
|
Change test limit for atime from 2ms to 5ms. Add comment explaining why the wiggle room is needed. Fixes: nodejs#24593
Change test limit for atime from 2ms to 5ms. Add comment explaining why the wiggle room is needed. Fixes: #24593 PR-URL: #30437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Change test limit for atime from 2ms to 5ms. Add comment explaining why the wiggle room is needed. Fixes: #24593 PR-URL: #30437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Change test limit for atime from 2ms to 5ms. Add comment explaining why the wiggle room is needed. Fixes: #24593 PR-URL: #30437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Change test limit for atime from 2ms to 5ms. Add comment explaining why the wiggle room is needed. Fixes: #24593 PR-URL: #30437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Hey, I just ran the test suit and it seems that the tests for
/parallel/test-fs-stat-bigint.js
fail about 3 times in 100 runs. When runningpython2 tools/test.py --repeat=100 -J parallel/test-fs-stat-bigint
the output is as follows:My system spec:
The text was updated successfully, but these errors were encountered: