-
Notifications
You must be signed in to change notification settings - Fork 971
Conversation
mrose17
commented
Aug 15, 2018
- catch “unlikely” but possible errors
- handle SIGHUP an dSIGINT
- catch “unlikely” but possible errors - handle SIGHUP an dSIGINT
@@ -104,29 +106,34 @@ const spawnGeth = async () => { | |||
} | |||
|
|||
const gethOptions = { | |||
stdio: process.env.GETH_LOG ? 'inherit' : 'ignore' | |||
stdio: ((envNet === 'ropsten') || process.env.GETH_LOG) ? 'inherit' : 'ignore' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this included intentionally? i think that would should let GETH_LOG
be the sole deciding indicator for logging, lest there is value in always having debug output on ropsten
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. it's intentional: if someone is on the test network, or someone turns on GETH_LOG, then we get logs, otherwise not. in other words, for production use, no logs by default; for testnet use, logs by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
process.on('exit', cleanupGeth) | ||
process.on('SIGHUP', cleanupGethAndExit) | ||
process.on('SIGTERM', cleanupGethAndExit) | ||
process.on('SIGINT', cleanupGethAndExit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for these ones ++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes work very well, thank you for hardening 👍