Skip to content
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

Capture Ctrl+C in cli_wallet when not in daemon mode #1193 #1232

Merged
merged 1 commit into from Oct 20, 2018

Conversation

Projects
5 participants
@cogutvalera
Copy link
Member

commented Aug 3, 2018

PR for #1193

@abitmore abitmore moved this from To do to In progress in Feature release (201810) Aug 3, 2018

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

IMO we should capture and treat SIGTERM the same as SIGINT.

@pmconrad
Copy link
Contributor

left a comment

Doesn't work for me - pressed CTRL-C and got message "Captured CTRL+C not in daemon mode", but wallet didn't exit.
I suspect that's because wallet_cli->stop internally calls the same wait() that is called by main.cpp, but https://github.com/bitshares/bitshares-fc/blob/master/src/thread/future.cpp#L107


fc::set_signal_handler([&wallet_cli, &wapi, &wallet_file](int signal) {
ilog( "Captured CTRL+C not in daemon mode" );
wapi->save_wallet_file(wallet_file.generic_string());

This comment has been minimized.

Copy link
@pmconrad

pmconrad Aug 9, 2018

Contributor

Not required here, will be handled in line 302

wapi->save_wallet_file(wallet_file.generic_string());
wallet_cli->stop();
std::terminate();

This comment has been minimized.

Copy link
@abitmore

abitmore Aug 14, 2018

Member

Terminate? Why not exit? (I want to know why but not simply asking for a change)

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Aug 14, 2018

Author Member

because exit cannot finish program fully as terminate can

This comment has been minimized.

Copy link
@abitmore

abitmore Aug 14, 2018

Member

I'm not convinced that we should call this. Actually, I don't like exit either, but IMHO terminate is even worse. For example, why not raise SIGKILL, I guess it can end the program as well, but all of them don't seem correct.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Aug 14, 2018

Author Member
# abort
indicates "abnormal" end to the program, and raises the the POSIX signal SIGABRT, which means that any handler that you have registered for that signal will be invoked, although the program will still terminate afterwords in either case. Usually you would use abort in a C program to exit from an unexpected error case where the error is likely to be a bug in the program, rather than something like bad input or a network failure. For example, you might abort if a data structure was found to have a NULL pointer in it when that should logically never happen.

# exit
indicates a "normal" end to the program, although this may still indicate a failure (but not a bug). In other words, you might exit with an error code if the user gave input that could not be parsed, or a file could not be read. An exit code of 0 indicates success. exit also optionally calls handlers before it ends the program. These are registered with the atexit and on_exit functions.

#std::terminate
is what is automatically called in a C++ program when there is an unhandled exception. This is essentially the C++ equivalent to abort, assuming that you are reporting all your exceptional errors by means of throwing exceptions. This calls a handler that is set by the std::set_terminate function, which by default simply calls abort.

This comment has been minimized.

Copy link
@abitmore

abitmore Aug 14, 2018

Member

We should do a "normal" end.

This comment has been minimized.

Copy link
@abitmore

abitmore Aug 14, 2018

Member

"normal" end won't fully terminate cli_wallet

This is what you need to figure out: how to end the program normally when needed.

Thanks.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Aug 14, 2018

Contributor

@cogutvalera I have explained above what the problem is, please try to solve instead of using dirty workarounds.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Aug 14, 2018

Author Member

@pmconrad I don't think terminate is dirty workaround, just didn't think as @abitmore mentioned about other wrappers that may use cli_wallet program, you didn't talk about wrappers and I didn't think about them

This comment has been minimized.

Copy link
@pmconrad

pmconrad Aug 14, 2018

Contributor

https://stackoverflow.com/questions/30250934/how-to-end-c-code

...but IMO exiting in any way is not the way to go here - just shut down wallet_cli cleanly.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Aug 14, 2018

Author Member

Thanks !

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

Guys, check please when you will have enough time. Is cancel thread good enough ?

Thanks !

@abitmore

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

I guess it's better, although the code looks a bit ugly.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

@abitmore Thank you very much ! I will think about it and how to improve the code !

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2018

Please explain why you don't shut down wallet_cli cleanly.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

@pmconrad please tell me what do you mean by shut down wallet_cli cleanly ?

do we have clean method that can close cli_wallet cleanly ? Direct me please and I will change the code with the best way and best approach of course for sure ! Not a problem, maybe I've missed something.

Thanks !

@abitmore

This comment has been minimized.

Copy link
Member

commented Aug 29, 2018

@cogutvalera I think we need code like wallet_cli->quit(); when captured SIGINT or SIGTERM in main(). I used the word like because I don't know if it will work, but it's the spirit.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2018

@abitmore @pmconrad I've tried earlier wapi->quit() and wallet_cli->stop() but it cannot stop thread from main when SIGINT or SIGTERM is captured, that's why currently I'm using pthread_cancel solution. Isn't it good enough ? Should I spent more time and fight with another one solution ? Should I try to find another one solution ?

Thanks !

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2018

3 weeks ago I wrote:

I suspect that's because wallet_cli->stop internally calls the same wait() that is called by main.cpp, but https://github.com/bitshares/bitshares-fc/blob/master/src/thread/future.cpp#L107

Try to work around this by adding a nowait=false parameter to stop, or something along that line.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2018

@pmconrad Thank you very much ! I've tried to work around earlier but nothing helped, will try again more to play with this approach as you suggested.

Thanks !

@abitmore abitmore removed this from In progress in Feature release (201810) Sep 14, 2018

@abitmore abitmore added this to In progress in Feature Release (201902) via automation Sep 14, 2018

@cogutvalera cogutvalera force-pushed the cogutvalera:issue_1193 branch from e054b81 to bed035f Sep 28, 2018

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2018

rebased
also #1193 issue requires PR for fc library bitshares/bitshares-fc#77

fc::set_signal_handler([&wallet_cli, &wapi, &wallet_file](int signal) {
ilog( "Captured SIGTERM not in daemon mode" );
wapi->save_wallet_file(wallet_file.generic_string());
wallet_cli->quit_blocked_thread();

This comment has been minimized.

Copy link
@pmconrad

pmconrad Sep 28, 2018

Contributor

I found a simpler solution: just call fclose(stdin);. Please test on Windows.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Sep 28, 2018

Author Member

Thanks ! Will check it ...

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Sep 28, 2018

Author Member

@pmconrad I've checked your method on Ubuntu 18.04 and your solution is simpler than mine I absolutely agree with you but your solution has disadvantage:

after unlocking wallet if you press CTRL+C then you cannot see what you type in command line, you need to restart it, in my approach we don't have this issue.

Don't you like my approach by cleanly quitting blocked thread ?

Thanks !

This comment has been minimized.

Copy link
@pmconrad

pmconrad Sep 30, 2018

Contributor

No, I don't like that. :-)

  • The thread may not be actually blocked at the time the signal is received. In that case your code will segfault.
  • When compiled with editline, there is a separate background thread active that will be left dangling with your method.

I could not reproduce the behaviour you described, but it might be a good idea to restore tty settings when cli shuts down.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Sep 30, 2018

Author Member

cli_wallet uses future and promise which has blocked_thread member, when SIGINT or SIGTERM is caught I use method quit of the blocked_thread member, thus we will stop cli_wallet's thread.

@pmconrad

  1. What do you mean by "The thread may not be actually blocked at the time the signal is received" ?
  2. My solution just quit only cli_wallet's thread, do you mean my solution may quit another thread instead of required ?
  3. To reproduce the behaviour I've described you should evaluate next 3 steps (tested on Ubuntu 18.04):
    3.1. unlock cli_wallet
    3.2 press CTRL+C
    3.3 try to write anything in command line tool and you won't see what are you typing there

Thanks !

This comment has been minimized.

Copy link
@pmconrad

pmconrad Sep 30, 2018

Contributor
  1. If the signal is received before you call wallet_cli->wait(), blocked_thread will be a nullptr. Admittedly this is unlikely in this specific case, but there may be other uses of fc::rpc::cli where it is less unlikely.
  2. No, there is another thread related to cli that is not stopped by your solution.
  3. I tried that (but not on Ubuntu). But never mind, if you see the behaviour then tty settings must be cleaned up. I suppose you do use editline? Can you reproduce this reliably?

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Oct 19, 2018

Author Member

I've used editline through VS code and after new updates seems all works fine enough now with your solution @pmconrad (I mean fclose(stdin) ! Very simple and efficient ! Thank you very much ! Will do new commit now and will close next PR in fc library bitshares/bitshares-fc#77 cause we don't need anymore

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

Please undo the fc bump in this PR and squash.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

ok sure ! Thanks !

@cogutvalera cogutvalera force-pushed the cogutvalera:issue_1193 branch from e4b59b7 to 7dc0de7 Oct 19, 2018

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

Done ! Thank you !

Feature Release (201902) automation moved this from In progress to Reviewer approved Oct 19, 2018

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

Thanks!

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

Thank you ! ;)

@abitmore

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

@pmconrad should we merge this for 201810 feature release? I think we can do it.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

IMO we shouldn't include further issues once we have published the testnet release. Except for critical issues and bugs that are caught during testing of course.
I think this one's not critical, therefore I'd postpone it until the next release.

@oxarbitrage

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

I agree with @pmconrad , some witnesses already installed new testnet version, announcements were made, etc. We should not push more changes to 201810 unless is absolutely necessary. We will always have stuff finished just one day after the release, we should left this and them out IMHO.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

Perhaps we should separate different releases in future for witness_node and cli_wallet

@pmconrad pmconrad merged commit 0eebc1e into bitshares:develop Oct 20, 2018

2 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Feature Release (201902) automation moved this from Reviewer approved to Done Oct 20, 2018

@pmconrad pmconrad referenced this pull request Oct 20, 2018

Closed

Capture Ctrl+C in cli_wallet when not in daemon mode #1193

6 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.