Conversation
rousskov
added
the
S-waiting-for-reviewer
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
I plan to come back to this PR after the backlog is cleared.
|
|
||
| /// \ingroup ServerProtocolICPAPI | ||
| void icpClosePorts(void); | ||
| /// Perform a graceful shutdown of ICP listening and sending ports (if any) |
There was a problem hiding this comment.
It is not clear to me what "graceful shutdown" implies in this context. Moreover, I do not think this function actually shuts down any ports -- the ports may continue to be open after this function returns. If fixing existing icpClosePorts() problems is outside this PR scope, then I recommend not adding any description of this problematic function in this PR.
| * to that specific interface. During shutdown, we must | ||
| * disable reading on the outgoing socket. | ||
| */ | ||
| assert(Comm::IsConnOpen(icpOutgoingConn)); |
There was a problem hiding this comment.
AFAICT, the removed assert checked an invariant that this function still uses. The assert did not check everything it should have checked, but do we have to remove it in this PR?
| void useConfig() override { icpOpenPorts(); } | ||
| void startReconfigure() override { icpClosePorts(); } | ||
| void syncConfig() override { icpOpenPorts(); } | ||
| void startShutdown() override { icpClosePorts(); } |
There was a problem hiding this comment.
Prior to this PR, SignalEngine::doShutdown() called icpConnectionShutdown() (via serverConnectionsClose()) but did not call icpClosePorts().
AFAICT, this PR changes the timing of Stop sending ICP from... event (i.e. icpOutgoingConn reset) during Squid shutdown. Resetting icpOutgoingConn at startShutdown() time makes it more difficult for still-active transactions to finish successfully during shutdown_lifetime. I do not think we should Stop sending ICP from... so early.
This alleged timing change was not disclosed in the PR description.
rousskov
removed
the
S-waiting-for-reviewer
2 participants
No description provided.