Run each console command in its own child process #3255

fainashalts · 2020-08-13T03:37:51Z

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.

packages/core/index.js

cds-amal · 2020-08-13T15:41:46Z

packages/core/lib/console.js

+      try {
+        this.provision();
+      } catch (e) {
+        console.log(e);
+      }
+    } catch (err) {
+      callback(err);
+    }


Is the intention to eat exceptions thrown by this.provision() and not invoke the callback on 160?

Unless I'm misunderstanding, the callback on lime 160 is for any exceptions thrown by spawning the repl. There's a try/catch for this.provision() and a try/catch for the spawning. Does that make sense or am I not following?

The intention is a bit hard to follow because the two catch expressions are next to each other. As it exists, if the provision after spawn fails then the code explicitly eats the error and continues execution to the next REPL prompt. Is an exception thrown by this.provision() benign as the code suggests?

this.provision(); can throw an Error (line 110) if something is awry with an artifact file. I think we should be invoke the callback in the outer catch block.

packages/core/lib/debug/interpreter.js

packages/core/lib/console-child.js

packages/core/lib/console.js

gnidan · 2020-08-16T03:11:23Z

Hey @fainashalts, check out the bundled version of Truffle - there's a deprecation notice upon every child process:

truffle(develop)> test
(node:96195) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
Using network 'develop'.

(On Node v10.22)

gnidan

Few more comments for you @fainashalts.

packages/core/lib/console.js

packages/core/lib/debug/interpreter.js

gnidan

Marking this as "request changes" until we can resolve that Buffer() error (or at least get to the bottom of it!)

gnidan · 2020-08-20T02:09:59Z

packages/core/lib/console-child.js

+const Web3 = require("web3");
+
+const inputStrings = process.argv[2];
+const network = process.argv[3];


Can we merge these two? i.e., if the user runs debug --network mainnet from inside truffle console --network development, then I would expect the debug command to connect to mainnet. This implementation suggests that behavior might be undefined (or favor the network specified by process.argv[3])

Seems like we shouldn't pass network separately as a positional argument, and instead pass it as a named --network option. Two options come to mind for how to do that:

Add logic to the parent process to merge --network=<network-name> into inputStrings, possibly by decoding inputStrings into a mapping + then re-encoding with { network } merged in.

Use a -- separator between parent-process-provided options and user-provided options, e.g.:

truffle(develop)> debug <tx-hash> --fetch-external ... would run command: $ console-child.js --network=develop -- debug <tx-hash> --fetch-external from parent ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ from user

Hi @gnidan so I don't think it's necessary to make this change - as far as I can tell, passing debug --network mainnet does run debug on mainnet even if truffle console was passed a different network. I tested with truffle console --network development and then debug <txhash> --network test, which I had running on a different port. The behavior is as expected. Let me know if that's not sufficient or if I'm missing something.

I'm actually not passing network to the run process, just in order to do a Config.detect() in order to get the develop network set up for the ganache url we pass to set up the provider (I know it's a separate consideration re: the hardcoded develop logic, which we've decided to address separately I believe).

If this is more of a concern re: having network as a positional argument from the parent, I can do a merge but it sort of seems unnecessary to me? Let me know what you think, thanks!

Hi @gnidan! #3296 addresses this in case you do want to get rid of the positional argument. Wanted to save you a little time!

Thanks so much @fainashalts, thanks for knocking that out!

That PR looks great! But why is it failing ☹️ ? (Re-running now)

…o handling

- Move runSpawn function to console.js file - Write console-child.js to handle child behavior - Serialize and deserialize config options where possible - Detect config again from child to avoid serialization issues for provider and resolver

…ss boundary

Pass network as a named --network option instead of positionally

gnidan

I think this is good to go!

@eggplantzzz I think when you do the release this week, you should enlist a bunch of us to participate in smoke-testing to give this the extra diligence it needs.

fainashalts requested review from gnidan and cds-amal August 13, 2020 03:38

fainashalts commented Aug 13, 2020

View reviewed changes

packages/core/index.js Show resolved Hide resolved

eggplantzzz mentioned this pull request Aug 13, 2020

Add warning about NodeJS 12 trufflesuite/trufflesuite.com#425

Closed

cds-amal reviewed Aug 13, 2020

View reviewed changes

packages/core/lib/debug/interpreter.js Show resolved Hide resolved

eggplantzzz reviewed Aug 13, 2020

View reviewed changes

packages/core/lib/console-child.js Outdated Show resolved Hide resolved

eggplantzzz reviewed Aug 13, 2020

View reviewed changes

packages/core/lib/console-child.js Outdated Show resolved Hide resolved

cds-amal reviewed Aug 13, 2020

View reviewed changes

packages/core/lib/console.js Outdated Show resolved Hide resolved

cds-amal reviewed Aug 13, 2020

View reviewed changes

packages/core/lib/console.js Outdated Show resolved Hide resolved

gnidan reviewed Aug 13, 2020

View reviewed changes

packages/core/lib/console.js Show resolved Hide resolved

fainashalts requested review from gnidan, eggplantzzz and cds-amal August 15, 2020 00:30

gnidan reviewed Aug 16, 2020

View reviewed changes

packages/core/lib/console.js Outdated Show resolved Hide resolved

packages/core/lib/debug/interpreter.js Show resolved Hide resolved

gnidan suggested changes Aug 16, 2020

View reviewed changes

gnidan reviewed Aug 20, 2020

View reviewed changes

gnidan changed the title ~~Spawn repl~~ Run each console command in its own child process Aug 24, 2020

fainashalts added 11 commits August 25, 2020 14:01

Add initial plumbing for spawned child process

d30cd56

Remove unneeded code, add spread operator for inputStrings

21ac67e

Change to spawnSync, remove ipc channel, remove some unnecessary stdi…

dd7b919

…o handling

Rework parent-child behavior:

9e4301f

- Move runSpawn function to console.js file - Write console-child.js to handle child behavior - Serialize and deserialize config options where possible - Detect config again from child to avoid serialization issues for provider and resolver

Rename child to spawnSync and add process exit code to child-console.js

73950ed

Rework console to use repl directly w/o ReplManager

a1a67cf

Rework debugger to use repl directly w/o ReplManager

f94f69e

Remove ReplManager code in repl.js

18b9ac9

Update path handling to ensure bundled version works

162efee

Add exit event handling; display prompt after child process exits

7bb248b

Declutter a little, and instantiate this.repl

5aef5d4

fainashalts added 6 commits August 25, 2020 14:01

Set context vars needed inside repl

25cd197

Remove test that looks for console since it is not longer exported

3496d8a

Add needed network info to spawn

9c928ba

Rename some variables, and stop passing networks objeect across proce…

8f6bc05

…ss boundary

Remove unused configNetworks object

cd5d68d

Add --no-deprecation flag to node spawn process; this is a stopgap

79ae210

fainashalts force-pushed the spawn-repl branch from c2b5cf3 to 79ae210 Compare August 25, 2020 21:01

fainashalts added 3 commits August 25, 2020 14:03

Pass network as a named --network option instead of positionally

f60754d

Use yargs to get and pass network value

a365736

Merge pull request #3296 from trufflesuite/spawn/network-input

eb33076

Pass network as a named --network option instead of positionally

gnidan approved these changes Aug 25, 2020

View reviewed changes

fainashalts merged commit e5cfd95 into develop Aug 25, 2020

fainashalts deleted the spawn-repl branch August 25, 2020 22:44

This was referenced Aug 26, 2020

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

Merged

Set context vars #3317

Merged

fainashalts restored the spawn-repl branch August 27, 2020 02:00

haltman-at deleted the spawn-repl branch June 26, 2023 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run each console command in its own child process #3255

Run each console command in its own child process #3255

fainashalts commented Aug 13, 2020 •

edited

cds-amal Aug 13, 2020

fainashalts Aug 14, 2020

cds-amal Aug 16, 2020

cds-amal Aug 25, 2020

gnidan commented Aug 16, 2020 •

edited

gnidan left a comment •

edited

gnidan left a comment

gnidan Aug 20, 2020 •

edited

fainashalts Aug 21, 2020 •

edited

fainashalts Aug 24, 2020

gnidan Aug 25, 2020

gnidan left a comment

Run each console command in its own child process #3255

Run each console command in its own child process #3255

Conversation

fainashalts commented Aug 13, 2020 • edited

cds-amal Aug 13, 2020

Choose a reason for hiding this comment

fainashalts Aug 14, 2020

Choose a reason for hiding this comment

cds-amal Aug 16, 2020

Choose a reason for hiding this comment

cds-amal Aug 25, 2020

Choose a reason for hiding this comment

gnidan commented Aug 16, 2020 • edited

gnidan left a comment • edited

Choose a reason for hiding this comment

gnidan left a comment

Choose a reason for hiding this comment

gnidan Aug 20, 2020 • edited

Choose a reason for hiding this comment

fainashalts Aug 21, 2020 • edited

Choose a reason for hiding this comment

fainashalts Aug 24, 2020

Choose a reason for hiding this comment

gnidan Aug 25, 2020

Choose a reason for hiding this comment

gnidan left a comment

Choose a reason for hiding this comment

fainashalts commented Aug 13, 2020 •

edited

gnidan commented Aug 16, 2020 •

edited

gnidan left a comment •

edited

gnidan Aug 20, 2020 •

edited

fainashalts Aug 21, 2020 •

edited