-
Notifications
You must be signed in to change notification settings - Fork 733
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
Copying fifos hangs. #727
base: master
Are you sure you want to change the base?
Copying fifos hangs. #727
Conversation
Unix I do see a difference in behavior: $ cp my_fifo some_file &
$ echo 'hi' > my_fifo # now write to the fifo
$ cat some_file # expected output
hi
# Using ShellJS
$ shx cp my_fifo some_file &
$ echo 'hi' > my_fifo
ShellJS: internal error
Error: ESPIPE: invalid seek, read
at Error (native)
at Object.fs.readSync (fs.js:731:19)
at copyFileSync (/home/nate/.npm-global/lib/node_modules/shx/node_modules/shelljs/src/cp.js:67:22)
at /home/nate/.npm-global/lib/node_modules/shx/node_modules/shelljs/src/cp.js:269:7
at Array.forEach (native)
at _cp (/home/nate/.npm-global/lib/node_modules/shx/node_modules/shelljs/src/cp.js:231:11)
at /home/nate/.npm-global/lib/node_modules/shx/node_modules/shelljs/src/common.js:338:25
at shx (/home/nate/.npm-global/lib/node_modules/shx/lib/shx.js:116:37)
at run (/home/nate/.npm-global/lib/node_modules/shx/lib/cli.js:20:23)
at Object.<anonymous> (/home/nate/.npm-global/lib/node_modules/shx/lib/cli.js:50:3)
$ cat some_file # empty file Maybe we should just fix the crash during copy? @freitagbr |
Hmm, intriguing.
eg:
So this PR isn't properly replicating cp, no... (Although, afaik, node doesn't support creating fifo's; except I guess if this shells out? :-s (Or uses some native bindings))
|
When copying the file directly (eg
vs the following for shelljs:
(The point being read vs pread; wherein (I'm just braindumping as I follow some of the rabbit hole :-) ) |
Okay, so I think I've fixed the scenario you've referenced :-) (Setting that offset to |
@bruce-one is there documentation on setting the offset to |
Ah, no, I just found it in https://github.com/libuv/libuv/blob/v1.x/src/unix/fs.c#L306 (and when looking through the libuv tests -1 is used for the offset quite a bit -- was trying to replicate it in straight uv and stumbled across the difference) |
That switch has been there for a very long time; and it kinda makes sense (I think?) because it's the way to say "no need to worry about offset, so just read not pread": libuv/libuv@74999f8#diff-6a16903c26af4b4035eda9922a73ecc9R160 (via a few steps in the middle) |
(Sorry for the noise; trying to fix the tests on Windows. It looks like Appvoyer's environment (cygwin?) supports |
Codecov Report
@@ Coverage Diff @@
## master #727 +/- ##
=========================================
+ Coverage 94.81% 97.4% +2.58%
=========================================
Files 33 33
Lines 1254 1231 -23
=========================================
+ Hits 1189 1199 +10
+ Misses 65 32 -33
Continue to review full report at Codecov.
|
test/cp.js
Outdated
@@ -797,7 +797,7 @@ test('should not attempt to copy fifos via symlinks in directories', t => { | |||
test('should copy fifos directly', t => { | |||
try { | |||
shell.exec(`mkfifo ${t.context.tmp}/fifo`); | |||
shell.exec(`echo -n test1 > ${t.context.tmp}/fifo`, { async: true }); | |||
shell.exec(`printf test1 > ${t.context.tmp}/fifo`, { async: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to see shx echo -n test1
here. Appveyor may have the command, but it may not be consistent with unix printf
, and I don't think it's available by default on Windows systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iirc, osx's echo doesn't support the -n
... (Which was why I changed it initially)
Also worth noting named pipes aren't supported directly by Windows (only via cygwin etc, which didn't appear reliable)
(Could match the \n
though if echo is preferred)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iirc, osx's echo doesn't support the -n... (Which was why I changed it initially)
shx echo
is platform agnostic, so it runs on OS X, Win, and Linux. The behavior is guaranteed to be the same across all machines, which is what you want out of an external dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah :-) good point :-)
Is it possible to easily shell out to shelljs's echo implementation? (I've shelled out here because the event loop is blocked by the cp, so need a way to get data into the fifo in the background; and atm this is shelling out to the /bin/sh impl (iirc?) and using it's echo/printf (although maybe there's a better way?)).
@bruce-one are we confident that |
From what I know/can ascertain (some but not extensive research), Windows can't put named pipes on a filesystem? (a la Hence support for this in native windows isn't possible? (Cygwin emulates pipes, but unless node is built for Cygwin, I don't think node will report them - from my testing.) I'm no Windows expert though... :-s |
Another outstanding question with this is in the recursive type scenario: should we call mkfifo or something to create the fifo to better replicate To illustrate:
|
Seems like the answer would be yes. Our goal here is compatibility with POSIX behavior. |
Shells out to `mknod`, so less platform independent than normal, although these devices are not platform independent.
Have added support for acting like From some reading (eg https://unix.stackexchange.com/questions/18712/difference-between-cp-r-and-cp-r-copy-command) Unfortunately creating character and block devices requires root permissions... I didn't know how to handle that. (Running ava with sudo and a grep for So for now those two tests just don't run on CI :-s (And I haven't even manually tested them on OSX...) |
@bruce-one thanks for following up on this
I would like to steer clear of root-only features in ShellJS core. After thinking a bit, your initial suggestion may have been right after all: skip these file types by default. We can indicate an error occurred via Since this is a great PR though, I think this would be a good candidate for a plugin. We're adding a plugin API for v0.8, allowing users to extend ShellJS. We could create a plugin to add back support for fifo and other specialized file types, overriding the default behavior. shell.cp('blockDeviceName', 'dest'); // default behavior
// cp: block device is not supported: 'blockDeviceName'
require('shelljs-plugin-cp-special-files');
shell.cp('blockDeviceName', 'dest');
// Now it succeeds! (assuming we're on Unix and have root permission) ShellJS proper won't need to have any more platform-dependent code, and we can still provide support for the rare users who need it. This sounds like a win. What do you think? |
src/cp.js
Outdated
args.push(major(stat.rdev), minor(stat.rdev)); | ||
} | ||
try { | ||
var cmd = exec(args.join(' '), { silent: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For security, I recommend child_process.execFileSync()
here. Just wrap it in a try-catch, since it throws an exception if the command fails.
try {
child_process.execFileSync('mknod', argsArray);
} catch (e) {
failureMessage = e.stderr;
}
src/cp.js
Outdated
var created; | ||
var failureMessage; | ||
var args = ['mknod']; | ||
args.push('"' + destFile + '"'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance this looks wrong. What if destFile
is hello world"; rm -rf *; echo "
? Looks like you'll be running:
$ mknod "hello world"; rm -rf *; echo "" p
JSON.stringify()
is the way we usually perform quote escaping, but you can avoid this entirely if you take my suggestion below.
src/cp.js
Outdated
@@ -153,6 +171,42 @@ function cpdirSyncRecursive(sourceDir, destDir, currentDepth, opts) { | |||
} // for files | |||
} // cpdirSyncRecursive | |||
|
|||
function major(rdev) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An explanatory comment might be good, since this is pretty niche and not very obvious
I've done that cleanup based on the feedback from your review (thanks! :-) ). I agree that making this a plugin does seem reasonable :-) Hence, should this PR be changed to just log the error message and then independently create a plugin package? Although, it might be nice to mention the plugin in the error/warning message? Hence maybe make the plugin, then change PR, then merge? Or are you not that keen on "centralising" plugins quite that much? (e.g. if it were called out explicitly (a la One of the reasons I ask that is because a "shelljs/plugin-posix" could be useful (from past experience) because I know I personally use shelljs solely on posix-like OSs, and find it's consistency excellent (and still very useful), but being able to do "all the posix things" in one plugin could be nice? |
Sounds like a good idea. Let's discuss the plugin over at #748. We'll leave this issue for only the cp-specific changes. |
This is flushed at the end of the copy command, or on exit if that doesn't happen first.
src/cp.js
Outdated
if (srcFileStat.isCharacterDevice()) type = 'block device'; | ||
if (srcFileStat.isBlockDevice()) type = 'character device'; | ||
common.error('copyFileSync: ' + type + ' is not supported (' + srcFile + ')', { continue: true }); | ||
common.logLater('copyFileSync: Special files were encountered during this operation. Please investigate shelljs plugins if you would like to add support for these.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the common.error()
log message is sufficient. Let's get rid of logLater
and move something like this into the docs. You can edit the //@
section in this file, and then run npm run gendocs
.
test/cp.js
Outdated
@@ -757,7 +757,7 @@ test('should not overwrite recently created files (not give error no-force mode) | |||
t.is(shell.cat(`${t.context.tmp}/file1`).toString(), 'test1'); | |||
}); | |||
|
|||
test('should copy fifos contents', t => { | |||
test('should copy fifo contents', t => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this case should also produce a warning. We'll completely drop fifo support and add it back in the plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fits in the "act like cp" sense.
Being, without the recursive
flag cp will treat these files as just "dumb files" rather than special files.
From what I'd read, that's the context of where this came from in posix's cp; i.e. originally it would just treat everything as a file and do a dumb copy (creating a normal-file rather than a special-file), but then later on support was added for being clever with special-files. Hence, my brain goes "native shelljs can treat them like normal-files" and "plugin'd shelljs gains the knowledge to treat them as special-files" parallels "normal cp is dumb" and "modern cp -R is smart". If that helps frame why I'd left this in?
There's a conflict between "act like cp" and "pull all the unix out" :-s
I was inclined to "act like cp" (and treat them like normal files, and hence hang while copying) because it's simple and native? Although that could just be me :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a conflict between "act like cp" and "pull all the unix out" :-s
I agree these goals are in conflict. I think in this case we should try to stay internally consistent. We should output a warning when copying any fifos and delegate to the plugin to provide actual posix behavior.
If the plugin aims for posix compatibility, what arguments does it need passed? It sounds like it needs options.recursive
, srcfile
, & destFile
, is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, sounds right :-) (Assuming the srcFile symlink has already been resolved if options.followsymlink === true
.) Perhaps it's worth passing the whole options
object though? (Thinking in terms of flexibility, eg if more flags are added (eg -p
))
(And I agree with your perspective on this: the whole reason I opened this PR in the first place was because it tripped me up, and staying internally consistent would have saved me whereas "acting like cp" wouldn't. Just hadn't quite come to that realisation yet :-p )
test/cp.js
Outdated
} | ||
}); | ||
|
||
test('should copy fifos contents via symlinks', t => { | ||
test('should copy fifo contents via symlinks', t => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, should this also be a warning?
This reverts commit 6a4a6a0.
(btw, more than happy to just log and not copy when in non-recursive mode, was just explaining why I'd originally left that aspect alone because it feels like sound logic (hence why I'd left it :-p ), but I'm more than happy make the change :-) ) |
shelljs'
cp
will hang if trying to copy a fifo.This PR skips copying more non-file-like entities to avoid this issue.