-
Notifications
You must be signed in to change notification settings - Fork 51
Wait before shutdown to avoid the crash #593
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
Conversation
codechain/codechain.yml
Outdated
| long: chain | ||
| help: Set the blockchain type out of solo, solo_authority, tendermint, cuckoo, blake_pow, husky or a path to chain scheme file. | ||
| takes_value: true | ||
| - wait-before-shutdown: |
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.
I don't think we need a command line option here because this PR is just a hotfix for the crash. Just use a hardcoded constant which is large enough to avoid the crash.
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.
It is to make it easy to test.
I'm not sure waiting 1 second is enough to avoid the crash. It was the experimental result on my machine.
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.
I think we need internal command line options to help test and debug.
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.
It is not a good idea to expose a command line option that is used only for test & debug purposes. What about adding an environment variable instead? Users don't know what environment variables exist, but we can use them for test & debug purposes.
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.
Okay. I agree with you. I'll change it.
This patch is the hotfix for #348. I can't find the exact reason for the SIGINT yet, but this patch fixes the crash.
|
@kseo PR updated. |
kseo
left a comment
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.
Ok
This patch is the hotfix for #348.
I can't find the exact reason for the SIGINT yet, but this patch fixes
the crash.