-
Notifications
You must be signed in to change notification settings - Fork 611
Manage ICP ports using a Runner #2365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@rousskov
rousskov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I plan to come back to this PR after the backlog is cleared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
No description provided.