trufflesuite / truffle-core

Core code for Truffle command line tool
MIT License
92 stars 93 forks source link

Close open handlers on exit #135

Closed cgewecke closed 6 years ago

cgewecke commented 6 years ago

Believe this safely resolves truffle 852 where migrations that rely on a connection to infura or other remote cluster fail to complete.

Ran examples of the problem with wtfnode which detects which handlers continue to run during a hang and saw both Timers (e.g. interval polling from web3 and provider-engine) and Sockets (provider-engine) being left open. These have close methods we can invoke on command exit.

Have also published this branch to darq-truffle@roderik and tested it against truffle develop and truffle console, everything works.

gnidan commented 6 years ago

Did you test this on, say, running debug inside truffle console? Not sure if things could get wonky in places where commands run inside other commands

cgewecke commented 6 years ago

I just checked it out and didn't see anything obviously amiss - the debugger runs and comes back to the shell without crashing. You might have a better sense of what to try. . .

Also checked multi-window, logged develop sessions running the debugger and everything seemed fine - could repeatedly run commands etc.

When you say 'command inside of other commands' do you mean one layer deep? Are the outer commands available in the debugger?

gnidan commented 6 years ago

I can only think of a hypothetical example, where console in the future might have some polling background handle or something, and running a command would kill that handle.

This is contrived, but in that case I could imagine this solution wouldn't work.

But the right answer is probably to make console commands run in their own processes anyway, so this should be fine.

cgewecke commented 6 years ago

@gnidan Agree - that's the only thing I can think of either and it seems like IPC isn't disturbed by this. Kind of a miracle.

cgewecke commented 6 years ago

Going to wait to get some feedback from truffle 852 before merging this anyway . . .