NuDBBackend destructor should not throw #4017
Conversation
| @@ -174,10 +183,20 @@ class NuDBBackend : public Backend | |||
| nudb::error_code ec; | |||
| db_.close(ec); | |||
| if (ec) | |||
| { | |||
| // Log to make sure the nature of the error gets to the user. | |||
| JLOG(j_.fatal()) << "NuBD close() failed: " << ec.message(); | |||
| Throw<nudb::system_error>(ec); | |||
There was a problem hiding this comment.
Since the error is getting logged, and the nearest catch block doesn't do anything with this error, what's the reason for still throwing? If we get rid of it seems like we could also eliminate the try/catch block in the destructor. This would also match the approach being used for the following call to remove_all.
There was a problem hiding this comment.
The reason I left the throw was so behavior is unchanged for any calls to close() other than the one from the destructor. I did not audit all calls to close(), but I assumed there were some because close() is a public method of Backend. Note the comment in Backend.cpp:
/** Close the backend.
This allows the caller to catch exceptions.
*/
virtual void
close() = 0;
There was a problem hiding this comment.
We would need to prove that no other client of close() is depending on the exception. This isn't impossible, but is a little tricky considering this is a public virtual.
There was a problem hiding this comment.
Preserving the existing throw sounds good
scottschurr
added
the
Passed
|
The CI failures are all known issues on macOS. So it looks like this code has survived as much of CI as possible at the moment. I'm marking the pull request passed. |
High Level Overview of Change
The
NuDBBackenddestructor currently may throw an exception. Most C++ folks agree that throwing an exception from a destructor is a bad idea:https://isocpp.org/wiki/faq/exceptions#dtors-shouldnt-throw
The modified code logs the problem that
NuDBis complaining about atfatallevel, so it is very likely to get into the log. Then it swallows the exception so the exception does not propagate out of the body of the destructor.Context of Change
There's been an unresolved sporadic crash only in certain testing scenarios where
NuDBBackendmight be implicated. While examining the code I noticed that theNuDBBackeddestructor could throw. So I'm patching that up and and improving the logging regarding what NuDB might be complaining about.Type of Change
There should be no behavior change as a result of this pull request. There's no reason to mention it in the release notes.