-
Notifications
You must be signed in to change notification settings - Fork 20.1k
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
cmd/geth, console: support interrupting the js console #23387
Conversation
Regarding that... Local console, eternal loop, hit
So, even for that case, this PR is better than master. What actually happens, though, is that since we're not in console reading mode any more, the |
This has now been improved. Local console, eternal loop, hit
|
4c552ce
to
90f5546
Compare
ping @gballet PTAL? |
I have some ideas for this, please don't merge it yet. |
@fjl was curious what your idea was regarding this PR |
I tried to implement the proper semantics where geth doesn't shut down on Ctrl-C. It's a bit complicated though. |
I was getting a crash immediately during start. I couldn't figure out where it came from but after a rebase against master the crash is gone |
I tried your idea here. It works. I thought sigterm should still cause shutdown. Do you agree with that? if so I need to change the code a bit because right now if first a sigint arrives and we're in console mode any future sigterm is ignored. Update: fixed sigterm handling here. So now ctrl+c only stops |
@holiman can you please take a look at my changes whenever you get a chance and tell me if its okay for me to push them to your branch? |
I find it a bit hard to follow, can't compare across forks, since you rebased mine before you committed yours. Do you think you can push the rebased to my repo for easier comparison? Edit: or squash your two commits into one? |
cmd: fix sigterm handling in console mode
Yes squashed: s1na@0e99d3f |
LGTM, push it here why dontcha? |
Yup, with your fixes, this now works for me:
|
I will re-check and merge later today! |
Better to pass this in from cmd/geth, which knows if it's running the console or not.
This reimplements the interrupt so that it always works. The previous implementation only worked for Interactive.
Fatalf doesn't shut down the node properly.
Calling it multiple times should not crash. Also move history writing into Interactive, where it belongs.
I've tested this extensively and decided to rework the interrupt logic a bit more to
|
LGTM. While testing this, I had some issues with the history, things didn't seem to be added to it. Then I cleared out the history (there were |
I need to recheck my change to the history write. Maybe it doesn't work properly now. |
I checked the history behavior and it seems to work. |
Previously, Ctrl-C (SIGINT) was ignored during JS execution, so it was not possible to get out of infinite loops in the console. With this change, Ctrl-C now interrupts JS. Fixes ethereum#23344 Co-authored-by: Sina Mahmoodi <itz.s1na@gmail.com> Co-authored-by: Felix Lange <fjl@twurst.com>
This PR changes the behavior of Ctrl-C (i.e. SIGINT) when the console is active.
Previously, this signal was ignored during JS execution, so it was not possible
to get out of infinite loops in the console. With this change, Ctrl-C now
interrupts JS.
Fixes #23344