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.
This is legacy code which would crash with assertion on shutdown when the ICP incoming and outgoing ports were the same.
This PR uses our current code style (used for other similar ports), which prevents these assertions from occuring by closing the out-port when it is open and leaving it as a reference to a closed port if it is a duplicate of the in-port.
- on shutdown any hanging reference will be dropped by the system.
- on reconfigure the reference will be unconditionally replaced with a new open one.
- we can be extra paranoid and move
icpOutgoingConn = nullptr;outside the new if-statement to free the memory early as-needed.
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.