Fix process disconnection #102
Conversation
|
I'm actually getting some exceptions now that the IPC channel is already disconnected with this change applied I'm gonna try to sort this out and re-open. |
|
Thanks for looking into this! It would definitely be great if we can get a reproducible test case one way or the other. |
|
So, this works (doesn't hang), but emits an error: ES2015: finish = () => setImmediate(process.disconnect);ES5: finish = function () {
return setImmediate(process.disconnect);
};This works and doesn't emit an error: ES2015: finish = () => setImmediate(() => process.disconnect());ES5: finish = function () {
return setImmediate(function () {
return process.disconnect();
});
};Seems like the first one should have worked, but it doesn't. Very strange. Anyway, I'm confident this works correctly now. I'm gonna try to find a good test case so someone else can verify. |
|
Alright, I found a reproducible test case: Check out React at commit find src -name "*.js" | head -77 | xargs ../jscodeshift/bin/jscodeshift.sh -v 2 -t ../app/js-codemod/transforms/no-vars.jsThis example will hang on The actual transformer used has no effect. I just used I imagine this is incredibly sensitive to the version of Node you're running (I'm on 5.6.0). Interestingly, this happens when I don't specify the I'm running BSD |
|
Notably, this problem can be solved instead by having the parent disconnect the worker: diff --git a/src/Runner.js b/src/Runner.js
index c4ff8b3..1eb0beb 100644
--- a/src/Runner.js
+++ b/src/Runner.js
@@ -231,6 +231,9 @@ function run(transformFile, paths, options) {
case 'free':
child.send({files: next(), options});
break;
+ case 'finish':
+ child.disconnect();
+ break;
}
});
return new Promise(resolve => child.on('disconnect', resolve));
diff --git a/src/Worker.js b/src/Worker.js
index e0e2a9e..779ae66 100644
--- a/src/Worker.js
+++ b/src/Worker.js
@@ -32,7 +32,7 @@ if (module.parent) {
return emitter;
};
} else {
- finish = () => { process.disconnect(); };
+ finish = () => process.send({action: 'finish'});
notify = (data) => { process.send(data); };
process.on('message', (data) => { run(data); });
setup(process.argv[2], process.argv[3]);Not sure which solution is better. |
|
@fkling Can you reproduce this error locally with the test case I described? |
|
Sorry, I didn't get to it yet. Will verify this tomorrow. |
|
Yep, I can reproduce this, but I'm still baffled about what happens here. It seems It's also unclear to me what the conditions are to trigger this issue. In react it seems passing ~75 causes the issue. On another codebase it's ~90 files. I will try to create a unit test for this and then merge this. |
|
@fkling Thanks for looking into this! Yeah, the bug is pretty strange in that it's not clear exactly what causes the problem. Anyway, I'm excited to see if you can create a unit test for this. |
I'm just generating 100 temp files ;) 668ed8d |
Update the createClass transform to insert a display name
I can consistently reproduce a problem with
jscodeshiftwhere all the transformations are applied correctly, but the process doesn't exit--it just hangs. It may be related to the problem that is described by @ForbesLindesay in #87.This is solved by forcing child workers to execute
process.disconnectat the beginning of their next event loop interation instead of during the event loop interation in which they are processing their final message. The relevant documentation is in the Node docs.Unfortunately, the files I've used to produce this behavior are part of a proprietary piece of software. I'll try to reproduce the bug on something I can publish. But until then, I should note that I'm able to do this with
-c 1(one worker), with fewer than 100 files.I'm running Node 5.6.0 on OS X 10.11.3.