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

Fix process disconnection #102

Merged
merged 2 commits into from Mar 27, 2016

Conversation

Projects
None yet
3 participants
@cjlarose
Contributor

cjlarose commented Mar 19, 2016

I can consistently reproduce a problem with jscodeshift where 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.disconnect at 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.

The 'disconnect' event will be emitted when there are no messages in the process of being received. This will most often be triggered immediately after calling child.disconnect().

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.

@cjlarose

This comment has been minimized.

Show comment
Hide comment
@cjlarose

cjlarose Mar 19, 2016

Contributor

I'm actually getting some exceptions now that the IPC channel is already disconnected with this change applied

TypeError: this.emit is not a function
    at [object Object].target.disconnect [as _onTimeout] (internal/child_process.js:637:12)
    at Timer.listOnTimeout (timers.js:92:15)
internal/child_process.js:637
      this.emit('error', new Error('IPC channel is already disconnected'));

I'm gonna try to sort this out and re-open.

Contributor

cjlarose commented Mar 19, 2016

I'm actually getting some exceptions now that the IPC channel is already disconnected with this change applied

TypeError: this.emit is not a function
    at [object Object].target.disconnect [as _onTimeout] (internal/child_process.js:637:12)
    at Timer.listOnTimeout (timers.js:92:15)
internal/child_process.js:637
      this.emit('error', new Error('IPC channel is already disconnected'));

I'm gonna try to sort this out and re-open.

@cjlarose cjlarose closed this Mar 19, 2016

@fkling

This comment has been minimized.

Show comment
Hide comment
@fkling

fkling Mar 19, 2016

Member

Thanks for looking into this! It would definitely be great if we can get a reproducible test case one way or the other.

Member

fkling commented Mar 19, 2016

Thanks for looking into this! It would definitely be great if we can get a reproducible test case one way or the other.

@cjlarose cjlarose reopened this Mar 19, 2016

@cjlarose

This comment has been minimized.

Show comment
Hide comment
@cjlarose

cjlarose Mar 19, 2016

Contributor

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.

Contributor

cjlarose commented Mar 19, 2016

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.

@cjlarose

This comment has been minimized.

Show comment
Hide comment
@cjlarose

cjlarose Mar 19, 2016

Contributor

Alright, I found a reproducible test case:

Check out React at commit 67f8524e88abbf1ac0fd86d38a0477d11fbc7b3e (at the time of writing, this is the latest commit).

find src -name "*.js" | head -77 | xargs ../jscodeshift/bin/jscodeshift.sh -v 2 -t ../app/js-codemod/transforms/no-vars.js

This example will hang on master, but will not hang after this fix is applied.

The actual transformer used has no effect. I just used no-vars.js from jscodemod as an example.

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 -c flag (running 7 workers on my 8-core machine), but also when I specify any value of -c from 1 to 7.

I'm running BSD find on OS X, so for the sake of reproducibility, let me enumerate those 77 files explicitly.

src/addons/__tests__/ReactComponentWithPureRenderMixin-test.js
src/addons/__tests__/ReactFragment-test.js
src/addons/__tests__/renderSubtreeIntoContainer-test.js
src/addons/__tests__/update-test.js
src/addons/link/__tests__/LinkedStateMixin-test.js
src/addons/link/__tests__/ReactLinkPropTypes-test.js
src/addons/link/LinkedStateMixin.js
src/addons/link/ReactLink.js
src/addons/ReactComponentWithPureRenderMixin.js
src/addons/ReactFragment.js
src/addons/ReactWithAddons.js
src/addons/renderSubtreeIntoContainer.js
src/addons/shallowCompare.js
src/addons/transitions/__tests__/ReactCSSTransitionGroup-test.js
src/addons/transitions/__tests__/ReactTransitionChildMapping-test.js
src/addons/transitions/__tests__/ReactTransitionGroup-test.js
src/addons/transitions/ReactCSSTransitionGroup.js
src/addons/transitions/ReactCSSTransitionGroupChild.js
src/addons/transitions/ReactTransitionChildMapping.js
src/addons/transitions/ReactTransitionEvents.js
src/addons/transitions/ReactTransitionGroup.js
src/addons/update.js
src/core/__tests__/ReactErrorBoundaries-test.js
src/isomorphic/children/__tests__/onlyChild-test.js
src/isomorphic/children/__tests__/ReactChildren-test.js
src/isomorphic/children/__tests__/sliceChildren-test.js
src/isomorphic/children/onlyChild.js
src/isomorphic/children/ReactChildren.js
src/isomorphic/children/sliceChildren.js
src/isomorphic/classic/__tests__/ReactContextValidator-test.js
src/isomorphic/classic/class/__tests__/ReactBind-test.js
src/isomorphic/classic/class/__tests__/ReactBindOptout-test.js
src/isomorphic/classic/class/__tests__/ReactClass-test.js
src/isomorphic/classic/class/__tests__/ReactClassMixin-test.js
src/isomorphic/classic/class/ReactClass.js
src/isomorphic/classic/element/__tests__/ReactElement-test.js
src/isomorphic/classic/element/__tests__/ReactElementClone-test.js
src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js
src/isomorphic/classic/element/ReactCurrentOwner.js
src/isomorphic/classic/element/ReactDOMFactories.js
src/isomorphic/classic/element/ReactElement.js
src/isomorphic/classic/element/ReactElementValidator.js
src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js
src/isomorphic/classic/types/ReactPropTypeLocationNames.js
src/isomorphic/classic/types/ReactPropTypeLocations.js
src/isomorphic/classic/types/ReactPropTypes.js
src/isomorphic/deprecated/OrderedMap.js
src/isomorphic/deprecated/ReactPropTransferer.js
src/isomorphic/devtools/ReactInvalidSetStateWarningDevTool.js
src/isomorphic/modern/class/__tests__/ReactClassEquivalence-test.js
src/isomorphic/modern/class/__tests__/ReactES6Class-test.js
src/isomorphic/modern/class/ReactComponent.js
src/isomorphic/modern/class/ReactNoopUpdateQueue.js
src/isomorphic/modern/element/__tests__/ReactJSXElement-test.js
src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js
src/isomorphic/modern/types/__tests__/ReactFlowPropTypes-test.js
src/isomorphic/modern/types/__tests__/ReactTypeScriptPropTypes-test.js
src/isomorphic/ReactDebugTool.js
src/isomorphic/ReactInstrumentation.js
src/isomorphic/ReactIsomorphic.js
src/React.js
src/ReactVersion.js
src/renderers/dom/__tests__/ReactDOMProduction-test.js
src/renderers/dom/client/__tests__/findDOMNode-test.js
src/renderers/dom/client/__tests__/ReactBrowserEventEmitter-test.js
src/renderers/dom/client/__tests__/ReactDOM-test.js
src/renderers/dom/client/__tests__/ReactDOMComponentTree-test.js
src/renderers/dom/client/__tests__/ReactDOMIDOperations-test.js
src/renderers/dom/client/__tests__/ReactDOMSVG-test.js
src/renderers/dom/client/__tests__/ReactDOMTreeTraversal-test.js
src/renderers/dom/client/__tests__/ReactEventIndependence-test.js
src/renderers/dom/client/__tests__/ReactEventListener-test.js
src/renderers/dom/client/__tests__/ReactMount-test.js
src/renderers/dom/client/__tests__/ReactMountDestruction-test.js
src/renderers/dom/client/__tests__/ReactRenderDocument-test.js
src/renderers/dom/client/__tests__/validateDOMNesting-test.js
src/renderers/dom/client/eventPlugins/__tests__/ChangeEventPlugin-test.js
Contributor

cjlarose commented Mar 19, 2016

Alright, I found a reproducible test case:

Check out React at commit 67f8524e88abbf1ac0fd86d38a0477d11fbc7b3e (at the time of writing, this is the latest commit).

find src -name "*.js" | head -77 | xargs ../jscodeshift/bin/jscodeshift.sh -v 2 -t ../app/js-codemod/transforms/no-vars.js

This example will hang on master, but will not hang after this fix is applied.

The actual transformer used has no effect. I just used no-vars.js from jscodemod as an example.

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 -c flag (running 7 workers on my 8-core machine), but also when I specify any value of -c from 1 to 7.

I'm running BSD find on OS X, so for the sake of reproducibility, let me enumerate those 77 files explicitly.

src/addons/__tests__/ReactComponentWithPureRenderMixin-test.js
src/addons/__tests__/ReactFragment-test.js
src/addons/__tests__/renderSubtreeIntoContainer-test.js
src/addons/__tests__/update-test.js
src/addons/link/__tests__/LinkedStateMixin-test.js
src/addons/link/__tests__/ReactLinkPropTypes-test.js
src/addons/link/LinkedStateMixin.js
src/addons/link/ReactLink.js
src/addons/ReactComponentWithPureRenderMixin.js
src/addons/ReactFragment.js
src/addons/ReactWithAddons.js
src/addons/renderSubtreeIntoContainer.js
src/addons/shallowCompare.js
src/addons/transitions/__tests__/ReactCSSTransitionGroup-test.js
src/addons/transitions/__tests__/ReactTransitionChildMapping-test.js
src/addons/transitions/__tests__/ReactTransitionGroup-test.js
src/addons/transitions/ReactCSSTransitionGroup.js
src/addons/transitions/ReactCSSTransitionGroupChild.js
src/addons/transitions/ReactTransitionChildMapping.js
src/addons/transitions/ReactTransitionEvents.js
src/addons/transitions/ReactTransitionGroup.js
src/addons/update.js
src/core/__tests__/ReactErrorBoundaries-test.js
src/isomorphic/children/__tests__/onlyChild-test.js
src/isomorphic/children/__tests__/ReactChildren-test.js
src/isomorphic/children/__tests__/sliceChildren-test.js
src/isomorphic/children/onlyChild.js
src/isomorphic/children/ReactChildren.js
src/isomorphic/children/sliceChildren.js
src/isomorphic/classic/__tests__/ReactContextValidator-test.js
src/isomorphic/classic/class/__tests__/ReactBind-test.js
src/isomorphic/classic/class/__tests__/ReactBindOptout-test.js
src/isomorphic/classic/class/__tests__/ReactClass-test.js
src/isomorphic/classic/class/__tests__/ReactClassMixin-test.js
src/isomorphic/classic/class/ReactClass.js
src/isomorphic/classic/element/__tests__/ReactElement-test.js
src/isomorphic/classic/element/__tests__/ReactElementClone-test.js
src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js
src/isomorphic/classic/element/ReactCurrentOwner.js
src/isomorphic/classic/element/ReactDOMFactories.js
src/isomorphic/classic/element/ReactElement.js
src/isomorphic/classic/element/ReactElementValidator.js
src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js
src/isomorphic/classic/types/ReactPropTypeLocationNames.js
src/isomorphic/classic/types/ReactPropTypeLocations.js
src/isomorphic/classic/types/ReactPropTypes.js
src/isomorphic/deprecated/OrderedMap.js
src/isomorphic/deprecated/ReactPropTransferer.js
src/isomorphic/devtools/ReactInvalidSetStateWarningDevTool.js
src/isomorphic/modern/class/__tests__/ReactClassEquivalence-test.js
src/isomorphic/modern/class/__tests__/ReactES6Class-test.js
src/isomorphic/modern/class/ReactComponent.js
src/isomorphic/modern/class/ReactNoopUpdateQueue.js
src/isomorphic/modern/element/__tests__/ReactJSXElement-test.js
src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js
src/isomorphic/modern/types/__tests__/ReactFlowPropTypes-test.js
src/isomorphic/modern/types/__tests__/ReactTypeScriptPropTypes-test.js
src/isomorphic/ReactDebugTool.js
src/isomorphic/ReactInstrumentation.js
src/isomorphic/ReactIsomorphic.js
src/React.js
src/ReactVersion.js
src/renderers/dom/__tests__/ReactDOMProduction-test.js
src/renderers/dom/client/__tests__/findDOMNode-test.js
src/renderers/dom/client/__tests__/ReactBrowserEventEmitter-test.js
src/renderers/dom/client/__tests__/ReactDOM-test.js
src/renderers/dom/client/__tests__/ReactDOMComponentTree-test.js
src/renderers/dom/client/__tests__/ReactDOMIDOperations-test.js
src/renderers/dom/client/__tests__/ReactDOMSVG-test.js
src/renderers/dom/client/__tests__/ReactDOMTreeTraversal-test.js
src/renderers/dom/client/__tests__/ReactEventIndependence-test.js
src/renderers/dom/client/__tests__/ReactEventListener-test.js
src/renderers/dom/client/__tests__/ReactMount-test.js
src/renderers/dom/client/__tests__/ReactMountDestruction-test.js
src/renderers/dom/client/__tests__/ReactRenderDocument-test.js
src/renderers/dom/client/__tests__/validateDOMNesting-test.js
src/renderers/dom/client/eventPlugins/__tests__/ChangeEventPlugin-test.js
@cjlarose

This comment has been minimized.

Show comment
Hide comment
@cjlarose

cjlarose Mar 19, 2016

Contributor

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.

Contributor

cjlarose commented Mar 19, 2016

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.

@cjlarose

This comment has been minimized.

Show comment
Hide comment
@cjlarose

cjlarose Mar 24, 2016

Contributor

@fkling Can you reproduce this error locally with the test case I described?

Contributor

cjlarose commented Mar 24, 2016

@fkling Can you reproduce this error locally with the test case I described?

@fkling

This comment has been minimized.

Show comment
Hide comment
@fkling

fkling Mar 25, 2016

Member

Sorry, I didn't get to it yet. Will verify this tomorrow.

Member

fkling commented Mar 25, 2016

Sorry, I didn't get to it yet. Will verify this tomorrow.

@fkling

This comment has been minimized.

Show comment
Hide comment
@fkling

fkling Mar 25, 2016

Member

Yep, I can reproduce this, but I'm still baffled about what happens here. It seems process.disconnect() is called just fine, but somehow the event doesn't make it the parent process. Or something happens in process.disconnect().

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.

Member

fkling commented Mar 25, 2016

Yep, I can reproduce this, but I'm still baffled about what happens here. It seems process.disconnect() is called just fine, but somehow the event doesn't make it the parent process. Or something happens in process.disconnect().

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.

@cjlarose

This comment has been minimized.

Show comment
Hide comment
@cjlarose

cjlarose Mar 25, 2016

Contributor

@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.

Contributor

cjlarose commented Mar 25, 2016

@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.

@fkling

This comment has been minimized.

Show comment
Hide comment
@fkling

fkling Mar 27, 2016

Member

I'm excited to see if you can create a unit test for this.

I'm just generating 100 temp files ;) 668ed8d

Member

fkling commented Mar 27, 2016

I'm excited to see if you can create a unit test for this.

I'm just generating 100 temp files ;) 668ed8d

@fkling fkling merged commit 5107ed2 into facebook:master Mar 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

euphocat pushed a commit to euphocat/jscodeshift that referenced this pull request Oct 22, 2017

Merge pull request #102 from acdlite/createclassdisplayname
Update the createClass transform to insert a display name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment