src: fix monerod shutdown hanging by making SIGINT handling async-signal-safe#10351
src: fix monerod shutdown hanging by making SIGINT handling async-signal-safe#10351navidR wants to merge 2 commits intomonero-project:masterfrom
Conversation
navidR
force-pushed
the
dev/navidr/fix-ctrl-c
branch
from
13c37b2 to
ec48d5e
Compare
| } | ||
| const auto remaining = deadline - now; | ||
| const auto wait_step = remaining < boost::posix_time::milliseconds(100) | ||
| ? remaining |
There was a problem hiding this comment.
This is:
wait_step = std::min(remaining, boost::posix_time::milliseconds(100));| bool r = local_shared_context->cond.timed_wait(lock, now + wait_step); | ||
| if(local_shared_context->ec == boost::asio::error::would_block && !r) | ||
| { | ||
| continue; |
There was a problem hiding this comment.
Why a continue at the end of the loop?
There was a problem hiding this comment.
Thanks. Fixed. That was a mistake.
src/common/util.cpp
Outdated
| } | ||
|
|
||
| std::function<void(int)> signal_handler::m_handler; | ||
| #ifndef WIN32 |
There was a problem hiding this comment.
We should switch to boost::asio::signal_set while we are hacking away in here. That way we don't need custom platform specific stuff. If we switch to it, we must switch all signal handlers to asio (thats the only catch).
I really don't want another thread for signal handling.
There was a problem hiding this comment.
I think your suggestion is correct. Therefore, I have switched to boost::asio::signal_set implementation right now.
Without any extra thread for signal handling.
navidR
force-pushed
the
dev/navidr/fix-ctrl-c
branch
from
ec48d5e to
fb69629
Compare
vtnerd
left a comment
vtnerd
left a comment
There was a problem hiding this comment.
I wish we could reuse existing io_context objects, but that is going to be more difficult. I will look into the overhead of each io_contect object...
src/common/util.cpp
Outdated
| std::function<void(int)> signal_handler::m_handler; | ||
| boost::mutex signal_handler::m_handler_mutex; | ||
| #ifndef WIN32 | ||
| std::shared_ptr<boost::asio::io_context> signal_handler::m_signal_io; |
There was a problem hiding this comment.
These two (m_signal_io and m_signal_set) should be combined into a single struct instead of allocating them separately.
There was a problem hiding this comment.
It might make sense to keep them separate if async_wait were capturing the signal_set, but it's not.
There was a problem hiding this comment.
Good suggestion; implemented.
src/common/util.h
Outdated
| #if defined(WIN32) | ||
| bool r = TRUE == ::SetConsoleCtrlHandler(&win_handler, TRUE); | ||
| if (r) | ||
| const bool r = TRUE == ::SetConsoleCtrlHandler(&win_handler, TRUE); |
There was a problem hiding this comment.
Why the separate variant for WIN32? boost::asio should handle all cross platform stuff with signal handlers
There was a problem hiding this comment.
Or I guess the benefit is the lack of asio needed to do the job?
There was a problem hiding this comment.
This one, I am hesitant about. Since replacing SetConsoleCtrlHandler with asio on Windows changes signal/control-event semantics.
src/common/util.h
Outdated
| static boost::mutex m_handler_mutex; | ||
|
|
||
| #ifndef WIN32 | ||
| static std::shared_ptr<boost::asio::io_context> m_signal_io; |
There was a problem hiding this comment.
Move both these variables to the cpp in the anonymous namespace. Then move the header includes to cpp too to reduce header bloat.
There was a problem hiding this comment.
Good suggestion; implemented.
navidR
force-pushed
the
dev/navidr/fix-ctrl-c
branch
from
fb69629 to
22e89a6
Compare
navidR
force-pushed
the
dev/navidr/fix-ctrl-c
branch
from
22e89a6 to
0928541
Compare
|
@vtnerd I added second commit to fix the DNS wait. |
|
Just look at the difference between fix and master: Fix: master: |
3 participants
This fix annoying issue of
Ctrl-Cnot working. Particularly happens when you are hitting theCtrl-Cat initialization.