Run each console command in its own child process #3319

fainashalts · 2020-08-27T02:09:43Z

Reverts #3314

We have a fix for this now, so we can revert the reversion and put in the fix, which is in #3317.

Original PR description:

This PR runs commands from inside the truffle develop and truffle console command processes. While this PR changes the way truffle develop and truffle console work under the hood, the end user CLI experience should not be different.

These changes became necessary when we found that Node v12 does not allow UncaughtException listeners in a REPL within another NodeJS program, and that one of our dependencies uses such a listener. By running a separate REPL in a child process we are able to bypass this problem. The issue that brought this to our attention is here: #2463

In the process of this work, it became apparent that the ReplManager class we had been using to manage multiple REPLs was no longer necessary since each REPL will now be in its own process. This PR thus also removes the repl.js file and directly accesses the node REPL as needed in console.js and interpreter.js.

To test this PR, run either truffle develop or truffle console and then use these to test out any other Truffle commands that make sense: migrate --reset, compile, test, debug, etc. Everything should work as it did previously, with the exception that the --network flag will throw an error when used with truffle develop -- the only network that should be used by truffle develop is the develop network.

…ocess

…me warnings we already saw in the parent

…ild process, and add comments

… creation

…tart function

Set context vars

packages/core/index.js

eggplantzzz · 2020-09-02T21:07:48Z

packages/truffle/webpack.config.js

@@ -40,26 +40,34 @@ module.exports = {
      "@truffle/core",
      "index.js"
    ),
+    consoleChild: path.join(


Just out of curiosity, why do we need to create a bundle for this? I guess it's because Truffle needs to use an external bundle to run the command in a separate process? It seems like this will drastically increase the size of the stuff we need to include with Truffle, huh?

The reason we have to create a bundle is to ensure that the console-child.js file can be properly accessed from the build version of truffle when I try to run it with the spawn. It's really just a path issue. Does that make sense?

I'm understanding that the bundle will be running this other bundle using the user's input. Yeah? This will make the distributed files a lot larger, right? I'm not trying to hold this PR up, I'm just curious.

I'm not sure I follow. The file being bundled is console-child.js, which is fairly short. The cli.bundled.js file is a tiny bit bigger -- 22.842243 megabytes on develop and 22.843334 megabytes on this branch. I don't think that's particularly significant?

More to the point though, how else would you get the correct path here?

Yes, but console-child.js is just the entry point. So it will probably almost double the size of the distributed stuff for Truffle. Essentially this is the same size as cli.bundled.js...which might be ok. I just wanted to point out that this seems to be a significant increase in size. Does that make sense?

Well I checked and it is not quite double, it increases the stuff in the build folder from ~50MB to like ~72MB. Something like that.

Hm, yeah I think that makes sense but I don't think there's another way. The whole point is that we can run Truffle commands through the child process. So we need all of that bundled this way. If you have a suggestion for how to reduce the distributed code size I'm happy to discuss further!

No it's probably fine. I just wanted to bring it up as something we should be aware of :)

Yeah, let's avoid having that bundle in two places :)

eggplantzzz · 2020-09-08T15:06:09Z

So we decided that the best fix here would be to define an entry point at core/lib/commands/index.js. That way we don't have to bundle cli.js and consoleChild.js. Those could be just copied to the dist folder and each could require the bundle. This is just an estimation of what would need to be done. There may be a little bit off messiness surrounding dealing with cli.js and consoleChild.js as I'm not sure if there is anything (processing-wise) that needs to be done with these files.

…ng on the context

Bundle commands separately

eggplantzzz · 2020-09-10T20:08:08Z

packages/core/lib/console.js

+    });
+  }
+
+  runSpawn(inputStrings, options, callback) {


We should rewrite this method to not take a callback. This does not hold up this PR from being merged, I just wanted to note that here.

eggplantzzz · 2020-09-10T20:09:25Z

packages/core/lib/debug/interpreter.js

@@ -321,7 +310,7 @@ class DebugInterpreter {

    //quit if that's what we were given
    if (cmd === "q") {
-      return await util.promisify(this.repl.stop.bind(this.repl))();
+      process.exit();


We should also eventually set the exit code here, no? Again, I don't think this should hold this PR up.

eggplantzzz

I like it!

fainashalts added 12 commits August 26, 2020 15:44

Add proper context handling to ensure artifacts available in child pr…

3a34db1

…ocess

Change stderr from parent to pipe because we don't need to see the sa…

67a0ff5

…me warnings we already saw in the parent

Fix context setting to ensure changes within repl are picked up by ch…

66f81a6

…ild process, and add comments

Revert "Revert "Run each console command in its own child process""

f0ee0f6

fix yargs to properly pass network

0750361

Remove console logs from earlier debugging

f711427

Replace setContextVars fx to use ES6 spread operators

217192b

Rework resetContractsInConsoleContext to preserve existing context vars

2186ab0

Remove spread operator use to ensure JS scripts persist between commands

bab017a

Remove this.repl check and move provisioning in start() to after repl…

6f13f08

… creation

Move repl.start outside of fetching accounts, move exit listener to s…

029d6b5

…tart function

Merge pull request #3317 from trufflesuite/set-context-vars

dc5bc5f

Set context vars

gnidan changed the title ~~Revert "Revert "Run each console command in its own child process""~~ Run each console command in its own child process Sep 2, 2020

gnidan mentioned this pull request Sep 2, 2020

Exception: running migrations and compile from Console #3327

Closed

eggplantzzz reviewed Sep 2, 2020

View reviewed changes

packages/core/index.js Show resolved Hide resolved

eggplantzzz reviewed Sep 2, 2020

View reviewed changes

fainashalts mentioned this pull request Sep 4, 2020

Remove subcommands architecture and mentions for merging later #3328

Merged

eggplantzzz and others added 4 commits September 9, 2020 11:19

Split code shared by cli.js and console-child.js out into its own bundle

be9b260

Add conditional logic to either use the raw JS or the bundles dependi…

eb8112c

…ng on the context

Merge pull request #3351 from trufflesuite/bundle-split

334b42d

Bundle commands separately

Merge branch 'develop' into revert-3314-revert-3255-spawn-repl

b5995da

eggplantzzz reviewed Sep 10, 2020

View reviewed changes

eggplantzzz approved these changes Sep 10, 2020

View reviewed changes

fainashalts merged commit 63d2e4a into develop Sep 10, 2020

fainashalts deleted the revert-3314-revert-3255-spawn-repl branch September 10, 2020 20:21

haltman-at mentioned this pull request Sep 11, 2020

Issue using debug in truffle develop #3205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run each console command in its own child process #3319

Run each console command in its own child process #3319

fainashalts commented Aug 27, 2020 •

edited by gnidan

eggplantzzz Sep 2, 2020 •

edited

fainashalts Sep 2, 2020

eggplantzzz Sep 3, 2020

fainashalts Sep 3, 2020

eggplantzzz Sep 3, 2020 •

edited

eggplantzzz Sep 3, 2020

fainashalts Sep 3, 2020

eggplantzzz Sep 4, 2020 •

edited

gnidan Sep 5, 2020

eggplantzzz commented Sep 8, 2020

eggplantzzz Sep 10, 2020

eggplantzzz Sep 10, 2020

eggplantzzz left a comment

Run each console command in its own child process #3319

Run each console command in its own child process #3319

Conversation

fainashalts commented Aug 27, 2020 • edited by gnidan

eggplantzzz Sep 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eggplantzzz Sep 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eggplantzzz Sep 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eggplantzzz commented Sep 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eggplantzzz left a comment

Choose a reason for hiding this comment

fainashalts commented Aug 27, 2020 •

edited by gnidan

eggplantzzz Sep 2, 2020 •

edited

eggplantzzz Sep 3, 2020 •

edited

eggplantzzz Sep 4, 2020 •

edited