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.

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

Investigate flaky test-fs-copyfile #15394

Closed
jasnell opened this issue Sep 13, 2017 · 20 comments · Fixed by #15745
Closed

Investigate flaky test-fs-copyfile #15394

jasnell opened this issue Sep 13, 2017 · 20 comments · Fixed by #15745
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.

Comments

@jasnell
Copy link
Member

jasnell commented Sep 13, 2017

Ping @cjihrig ... seeing some flakiness in the test. This is the second time I've seen this running locally on Ubuntu.

=== release test-fs-copyfile ===
Path: parallel/test-fs-copyfile
assert.js:43
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 33188 === 33204
    at verify (/home/james/node/main/test/parallel/test-fs-copyfile.js:17:10)
    at Object.<anonymous> (/home/james/node/main/test/parallel/test-fs-copyfile.js:32:1)
    at Module._compile (module.js:600:30)
    at Object.Module._extensions..js (module.js:611:10)
    at Module.load (module.js:521:32)
    at tryModuleLoad (module.js:484:12)
    at Function.Module._load (module.js:476:3)
    at Function.Module.runMain (module.js:641:10)
    at startup (bootstrap_node.js:201:16)
    at bootstrap_node.js:626:3
Command: out/Release/node /home/james/node/main/test/parallel/test-fs-copyfile.js
@mscdex mscdex added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. labels Sep 13, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Sep 13, 2017

It looks like the reported file permissions are not identical. @jasnell when you see this error, are you able to verify if the copied file has the correct permissions? I'm curious if this is a bug in the copy code, or a bug in the test.

@addaleax
Copy link
Member

I can reproduce this; the files do get different permissions. This should be the relevant output:

mkdir("/home/sqrt/src/node/test/tmp", 0777) = 0
open("/home/sqrt/src/node/test/tmp/copyfile.out", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = 12
close(12)                               = 0
open("/home/sqrt/src/node/test/fixtures/a.js", O_RDONLY|O_CLOEXEC) = 12
fstat(12, {st_mode=S_IFREG|0644, st_size=1469, ...}) = 0
open("/home/sqrt/src/node/test/tmp/copyfile.out", O_WRONLY|O_CREAT|O_CLOEXEC, 0100644) = 13
sendfile(13, 12, [0] => [1469], 1469)   = 1469
close(12)                               = 0
close(13)                               = 0
open("/home/sqrt/src/node/test/fixtures/a.js", O_RDONLY|O_CLOEXEC) = 12
fstat(12, {st_mode=S_IFREG|0644, st_size=1469, ...}) = 0
read(12, "// Copyright Joyent, Inc. and ot"..., 1469) = 1469
close(12)                               = 0
stat("/home/sqrt/src/node/test/fixtures/a.js", {st_mode=S_IFREG|0644, st_size=1469, ...}) = 0
open("/home/sqrt/src/node/test/tmp/copyfile.out", O_RDONLY|O_CLOEXEC) = 12
fstat(12, {st_mode=S_IFREG|0664, st_size=1469, ...}) = 0
read(12, "// Copyright Joyent, Inc. and ot"..., 1469) = 1469
close(12)                               = 0
stat("/home/sqrt/src/node/test/tmp/copyfile.out", {st_mode=S_IFREG|0664, st_size=1469, ...}) = 0

I’m not seeing any part of the copy code that would make sure the target file gets the original file mode.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 13, 2017

Thanks @addaleax.

I’m not seeing any part of the copy code that would make sure the target file gets the original file mode.

That is done (or should be done) in libuv here. That is why you see the fstat() call before the third open() call.

@addaleax
Copy link
Member

@cjihrig Yes, but that only applies to the case in which the file is created, not the one in which it is overwritten (i.e. the one in the test file)

@cjihrig
Copy link
Contributor

cjihrig commented Sep 13, 2017

Ah. Yea, it looks like a fchmod() is needed.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 13, 2017

@addaleax does this commit fix the problem for you?

@addaleax
Copy link
Member

@cjihrig Looks that way 👍 And makes sense, too. I think both behaviours could make sense depending on the actual use case, though…

@cjihrig
Copy link
Contributor

cjihrig commented Sep 13, 2017

I think both behaviours could make sense depending on the actual use case, though…

What use cases do you have in mind?

@addaleax
Copy link
Member

@cjihrig I think I remember cases where I had applications write history files (à la .bash_history), which implemented that by writing to a new file, then copying at the end of the session instead of just appending to the original file, which always accidentally made the file mode match the one the temp file got by default rather than the one I explicitly gave to that history file…

idk, it was nothing more than a mild annoyance. I guess all I’m saying is, sometimes people give their files a certain mode for a reason, and sometimes (a lot of the time?) copying should respect that, since the semantics of a file and therefore its security properties are usually defined via its path, not where the contents were copied from.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 13, 2017

I agree that copying should retain the original mode. You said both behaviours could make sense. I guess my question is, when would you not want to copy the mode in a standard copy operation? I know Apple's copyfile() lets you copy different aspects of the file, such as data only, but for a basic cross platform cp operation, I think we'd always want everything.

@addaleax
Copy link
Member

@cjihrig Uh, which one are you agreeing with? The problem is that there can be two different “original” file modes, and there is no generic way to tell which mode is the desired one.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 13, 2017

I'm only referring to the case of cp src dest, where dest should have the same mode as src. I think we're on the same page, just failing to communicate it properly :-)

@addaleax
Copy link
Member

Yeah, maybe to use more technical terms, I think sometimes you want the effect of cp src dest and sometimes you want effect of cat < src > dest

@cjihrig
Copy link
Contributor

cjihrig commented Sep 13, 2017

Ok, now we're clear. Yea, I'm not currently trying to solve for cat < src > dest. I think if there were enough demand for it, it would be fairly straightforward to add that in the future though.

@refack refack added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Sep 14, 2017
cjihrig added a commit to cjihrig/libuv that referenced this issue Sep 14, 2017

Verified

This commit was signed with the committer’s verified signature.
cjihrig Colin Ihrig
This commit introduces fchmod() in uv_fs_copyfile() to set the
mode of the destination file.

Refs: nodejs/node#15394
PR-URL: libuv#1547
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
@trevnorris
Copy link
Contributor

This test isn't flaky, it's platform dependent and has been failing 100% of the time for me for the last three weeks. Since this feature exists on v8.x branch it should either be fixed soon or be removed until it can be fixed.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 24, 2017

It should be fixed by the next libuv update.

@brycebaril
Copy link
Contributor

This is a umask issue with the test, it expects 0022 for the test to pass, fails with 0002

@mscdex
Copy link
Contributor

mscdex commented Oct 2, 2017

Any ETA on next libuv release? I also see this every time when running tests (on Linux).

@cjihrig
Copy link
Contributor

cjihrig commented Oct 2, 2017

Aiming for tomorrow or the day after. You can try applying libuv/libuv@eaf25ae locally and see if it solves the problem for you.

@mscdex
Copy link
Contributor

mscdex commented Oct 2, 2017

@cjihrig Yes, that seems to fix it for me.

cjihrig added a commit to cjihrig/node that referenced this issue Oct 5, 2017

Verified

This commit was signed with the committer’s verified signature.
cjihrig Colin Ihrig
Refs: nodejs#15380
Refs: nodejs#15683
Fixes: nodejs#15394
Fixes: nodejs#15770
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Trott pushed a commit to Trott/io.js that referenced this issue Oct 5, 2017
PR-URL: nodejs#15745
Refs: nodejs#15380
Refs: nodejs#15683
Fixes: nodejs#15394
Fixes: nodejs#15770
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Oct 7, 2017

Unverified

The email in this signature doesn’t match the committer email.
PR-URL: #15745
Refs: #15380
Refs: #15683
Fixes: #15394
Fixes: #15770
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Oct 11, 2017

Unverified

The email in this signature doesn’t match the committer email.
PR-URL: #15745
Refs: #15380
Refs: #15683
Fixes: #15394
Fixes: #15770
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to addaleax/ayo that referenced this issue Oct 12, 2017

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
PR-URL: nodejs/node#15745
Refs: nodejs/node#15380
Refs: nodejs/node#15683
Fixes: nodejs/node#15394
Fixes: nodejs/node#15770
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Oct 17, 2017

Unverified

The email in this signature doesn’t match the committer email.
PR-URL: #15745
Refs: #15380
Refs: #15683
Fixes: #15394
Fixes: #15770
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Oct 25, 2017

Verified

This commit was signed with the committer’s verified signature.
MylesBorins Myles Borins
PR-URL: #15745
Refs: #15380
Refs: #15683
Fixes: #15394
Fixes: #15770
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants