Do not broadcast already canceled signals - #331
Conversation
Before this improvement each signal state was represented with a pair global variables (e.g., do_shutdown and ShutdownSignal). This approach is difficult to maintain and error-prone, because the variables could be easily brought out of sync. Now each signal state is encapsulated in a Signal object and Signals class manages all received signals.
| sigusr2_handle(int sig) | ||
| { | ||
| static int state = 0; | ||
| /* no debugs() here; bad things happen if the signal is delivered during _db_print() */ |
There was a problem hiding this comment.
This function explicitly calls debugs(). It may also indirectly call debugs() via Debug::parseOptions(). We should either add an XXX or refactor to address that old problem. Since this problem is outside this PR scope, I suggest adding an XXX:
| /* no debugs() here; bad things happen if the signal is delivered during _db_print() */ | |
| // no debugs() here; bad things happen if the signal is delivered during _db_print() | |
| // XXX: Refactor to remove direct and indirect debugs() calls. |
| #if !HAVE_SIGACTION | ||
| /* reinstall */ | ||
| if (signal(sig, sigusr2_handle) == SIG_ERR) { | ||
| int xerrno = errno; | ||
| debugs(50, DBG_CRITICAL, "signal: sig=" << sig << " func=sigusr2_handle: " << xstrerr(xerrno)); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
This and similar code in other handlers should be moved into the PR-added add() or similar method AFAICT -- all handlers need to reinstall the handler (where signals cancel previous installation). Since we are adding add() calls to all handlers, we should remove this old (and problematic) code duplication.
| pid = WaitForAnyPid(status, waitFlag); | ||
| getCurrentTime(); | ||
|
|
||
| // check for a stopped kid |
There was a problem hiding this comment.
| // check for a stopped kid | |
| // check for a stopped kid |
| if (TheSignals().on(Signal::Role::Reconfigure)) { | ||
| if (!TheSignals().avoidAction(Signal::Role::Reconfigure)) |
There was a problem hiding this comment.
All these on() calls are followed by avoidAction() checks. We should merge the two. Most likely, we should also move the signal handling action itself into that method, so that we end up with a loop across all TheSignals(). Something along these lines:
for (auto &signal: TheSignals()) {
signal.checkpoint();
}or, if really necessary (for reasons other than "performance"),
for (auto &signal: TheSignals()) {
if (signal.checkpoint())
return;
}There was a problem hiding this comment.
Added checkpoint(). I did not add iterators to make the range loop as you suggested. We can add this sugar if you insist.
| startAction(); | ||
|
|
||
| if (action.doBroadcast_) | ||
| action.broadcast(); |
There was a problem hiding this comment.
AFAICT, the above code is all about a single action. It should be moved into a dedicated action.masterCheckpoint() method, similar to the checkpoint() method mentioned elsewhere in this review.
| signalVar = 0; | ||
| actions_[role].clear(); | ||
| } | ||
| else if (Instance::Starting()) { |
There was a problem hiding this comment.
Eduard at #331 (review): I found another problem - the master process does not do anything to switch
Instance::Starting()into a 'not starting' mode. The PR code as it is does not work: sending one HUP signal (without INT) causes an endless loop printing thedelaying reconfiguration request during startupmessage (becauseAvoidSignalAction()always goes into theelse if (Instance::Starting())path in this scenario.
Noted. The same problem exists in base/bag51 code, right?
Perhaps we need to set
TheStartup().endedto true somewhere in watch_child() (after all kinds kicked in) and add some API to Instance.h for this.
What activities are keeping startup from ending in this case? Ideally, we want activities to end properly rather than forcing TheStartup().ended to become true.
There was a problem hiding this comment.
The same problem exists in base/bag51
yes
What activities are keeping startup from ending in this case?
Instance::Startup::confirmationCheckpoint() that resets Startup::ended does not start for Squid master process because there are no activities in that process.
| signalVar = 0; | ||
| actions_[role].clear(); | ||
| } | ||
| else if (Instance::Starting()) { |
There was a problem hiding this comment.
The same problem exists in base/bag51
yes
What activities are keeping startup from ending in this case?
Instance::Startup::confirmationCheckpoint() that resets Startup::ended does not start for Squid master process because there are no activities in that process.
| if (TheSignals().on(Signal::Role::Reconfigure)) { | ||
| if (!TheSignals().avoidAction(Signal::Role::Reconfigure)) |
There was a problem hiding this comment.
Added checkpoint(). I did not add iterators to make the range loop as you suggested. We can add this sugar if you insist.
| sigusr2_handle(int sig) | ||
| { | ||
| static int state = 0; | ||
| /* no debugs() here; bad things happen if the signal is delivered during _db_print() */ |
| #if !HAVE_SIGACTION | ||
| /* reinstall */ | ||
| if (signal(sig, sigusr2_handle) == SIG_ERR) { | ||
| int xerrno = errno; | ||
| debugs(50, DBG_CRITICAL, "signal: sig=" << sig << " func=sigusr2_handle: " << xstrerr(xerrno)); | ||
| } | ||
| #endif |
| startAction(); | ||
|
|
||
| if (action.doBroadcast_) | ||
| action.broadcast(); |
| pid = WaitForAnyPid(status, waitFlag); | ||
| getCurrentTime(); | ||
|
|
||
| // check for a stopped kid |
* Squid master process never left the 'startup' mode * Reconfiguration during another reconfiguration caused level-1 errors The solution for the (2) problem is not complete: added an XXX. Also adjusted debugging to be in line with the Daft test.
2 participants
No description provided.