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.
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.
In other words; the global port references are set to "closed" (for new activity), while the ports themselves are left to be closed "gracefully" (via RAII) when the last active transaction using them is done.
| * 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?
There was a problem hiding this comment.
The invariant I see here is that the outgoing port is already closed. Either by being the same object used by incoming port, or having closed already (eg by that read handler getting a 0-byte read).
What else do you think also needs to be checked in order to close the outgoing port?
given that this logic is now being run on starting shutdown/reconfigure.
rousskov
removed
the
S-waiting-for-reviewer
Delay closing the ICP ports until grace period for client transactions has ended. On FATAL/death/abort events do not close, which matches other port handling cleanly and leaves port information in core dumps in case it is relevant.
yadij
added
the
S-waiting-for-reviewer
2 participants
No description provided.