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.
3 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.