proxy-client: tolerate exceptions from remote destroy during cleanup#273
proxy-client: tolerate exceptions from remote destroy during cleanup#273xyzconstant wants to merge 2 commits intobitcoin-core:masterfrom
Conversation
Reproduces bitcoin-core#219 by saving a callback on the server then disconnecting before cleanup completes, so the server-side `~ProxyClient` runs its remote destroy call against a dead connection. Without the fix in the next commit, the throw escapes the noexcept destructor and aborts mptest.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 6de92e1. Nice fix and test.
Similar to #260, I think it is reasonable to suppress and log exceptions here and better than the alternative of not handling them correctly. But I think the best thing to do would be to propagate the exceptions to callers.
In issue #219 the choice of whether to throw or suppress the error here is a little muddy because #219 is a case where a server object (handler) owns a client object (notifications), and the exception happens when the server object is being garbage collected and trying to delete the client object. In this case, since the error happens during garbage collection, there's not really anything for server to do better than logging the failure.
But after this change, all errors that happen when trying to destroy client objects will be suppressed, and that is probably not ideal. It's just better than the current behavior.
There was a problem hiding this comment.
Code review ACK 6de92e1.
This looks like a good fix to me. Catching the exception around the remote destroy() call in the cleanup path makes sense here since ~ProxyClientBase is noexcept, and the added regression test covers the reported failure mode clearly.
Similar to #260, I think it is reasonable to suppress and log exceptions here and better than the alternative of not handling them correctly. But I think the best thing to do would be to propagate the exceptions to callers.
In issue #219 the choice of whether to throw or suppress the error here is a little muddy because #219 is a case where a server object (handler) owns a client object (notifications), and the exception happens when the server object is being garbage collected and trying to delete the client object. In this case, since the error happens during garbage collection, there's not really anything for server to do better than logging the failure.
I looked into what it would take to propagate the destroy-time exception instead of suppressing it here, along the lines of what @ryanofsky mentioned.
The straightforward version of that seems to require making ProxyClientBase destruction itself throwable so the cleanup failure can escape to the caller.
I tried that locally, and while it can be made to build, it broadens the change quite a bit by allowing ordinary client destruction to throw in teardown paths that are not generally in a good position to recover.
Given that, I think the current approach is the better tradeoff for this path.
4 participants
Fixes #219.
Catches exceptions from
Sub::destroy(*this)in~ProxyClientBase's cleanup lambda so a failed remote destroy call doesn't crash the process. Regression test included.