Clean up in Child_process module #2463
Conversation
| @@ -442,7 +442,7 @@ ChildProcess.prototype.spawn = function(options) { | |||
|
|
|||
| this._internal.close(); | |||
| this._internal = null; | |||
| throw errnoException(errno, 'spawn'); | |||
| throw errnoException('EINVAL', 'spawn', 'Could not execute'); | |||
There was a problem hiding this comment.
Why this change? Is errno unset or garbage?
There was a problem hiding this comment.
@bnoordhuis I have no problem with a errno var, but where is it created, really confusing :/
There was a problem hiding this comment.
It's created and set in C++ land, see https://github.com/joyent/node/blob/5b05429/src/process_wrap.cc#L199
There was a problem hiding this comment.
@bnoordhuis Ah, okay, well I will restore that an make a comment.
Do we have a magic errno to: https://github.com/joyent/node/pull/2463/files#L0R51
There was a problem hiding this comment.
Do we have a magic errno to: https://github.com/joyent/node/pull/2463/files#L0R51
Yes. (I was actually just working on a small patch for v0.6 that updated that line of code when I saw your PR).
|
Also, isn't this a dup of #2462? |
|
@bnoordhuis yes it is, but I have already closed it. I messed up when creating that pull request. |
| @@ -497,6 +497,8 @@ ChildProcess.prototype.kill = function(sig) { | |||
| if (this._internal) { | |||
| this.killed = true; | |||
| var r = this._internal.kill(signal); | |||
| // TODO: raise error if r == -1? | |||
| if (r === -1) { | |||
| throw errnoException('ESRCH', 'kill', 'No such process'); | |||
There was a problem hiding this comment.
Do we have a magic errno to this?
There was a problem hiding this comment.
Yes. All bindings set errno on error. If they don't, it's probably a bug.
|
@bnoordhuis restored errno magic. |
|
@bnoordhuis could we land this :) |
|
@AndreasMadsen: Is this patch targeted at v0.6 or master? If it's v0.6, can you revert the change that makes Also, can you |
|
@bnoordhuis This is targeted v0.6, I have splited this up in 4 logical commits and removed the |
| @@ -304,7 +306,6 @@ exports.execFile = function(file /* args, options, callback */) { | |||
| } | |||
|
|
|||
| child.stdout.setEncoding(options.encoding); | |||
| child.stderr.setEncoding(options.encoding); | |||
There was a problem hiding this comment.
Oh, by first look I thought this was just a line dublicate, this should not be deleted.
|
@bnoordhuis this is now fixed, tested and check - lets land this. |
|
Note this has also been discussed at the IRC: http://piscisaureus.no.de/log/2012-01-13#20:06:04.073 |
| // TODO make this more compatible with ErrnoException from src/node.cc | ||
| // Once all of Node is using this function the ErrnoException from | ||
| // src/node.cc should be removed. | ||
| var e = new Error(syscall + ' ' + errorno); | ||
| var message = errorno + ', ' + msg + (path ? ' \'' + path + '\'' : ''); |
There was a problem hiding this comment.
Turn the ternary operator into an if statement, this is hard to read.
In case of a write failure when using .fork an error would be throw, however the errnoExecption was not used here makeing it extreamly difficute to detect. This is a bug since errno was used, but it was in the Error message only.
|
@bnoordhuis Here you go, as minimal as it can ever be. |
|
Landed in ca6eded. Thanks @AndreasMadsen. |
This is not really a patch but more like an issue
After have been hacking the
child_processmodule for quite some time I have found some wired stuff.I have tried to make sense of it in this patch. I think got most of it but I do not understand the magic errno variable there suddenly exist.