Skip to content
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

Build warnings - throw within destructor #1246

Closed
jmjatlanta opened this issue Aug 10, 2018 · 4 comments

Comments

@jmjatlanta
Copy link
Contributor

commented Aug 10, 2018

There are a few areas where throw is being called within a destructor. I believe the code "does the right thing", but the multitude of compiler warnings hide other errors/warnings that may be problems. These should be examined to see if they are doing the right thing, and to see if it can be modified to prevent warnings.

The two biggest culprits seem to be

  1. undo_database.hpp ~session()
  2. fc/exception/exception.hpp FC_RETHROW_EXCEPTIONS

Build Environment

  • Host OS: Ubuntu 18.04 LTS
  • Host Physical RAM: 16GB
  • Source Branch/Tag: develop
  • OpenSSL Version: 1.1.0h
  • Boost Version: 1.65.1
  • C++ Compiler: g++ version 7.3.0

Steps To Reproduce
cmake .
make

CORE TEAM TASK LIST

  • Evaluate Build Error
  • Provide build guidance
  • Create Bug Report
@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2019

Here is a discussion of pros and cons of throwing in a destructor. I believe most would agree that this is a bad idea, and can lead to undefined behavior.

There are a few places within Bitshares where a destructor can throw. The most prominent example is the session destructor of undo_db. I have reduced the number of warnings displayed by moving the implementation of the destructor from the .hpp to the .cpp.

We must decide what is the desired behavior in this case. We are attempting to clean up the allocations of the session, and something bad happened. Currently, terminate() is called, as per the c++ specifications. I believe this to be the desired behavior. But a warning is generated. To avoid the warning, I believe we should remove the throw and explicitly call std::terminate(). I believe the implications are minimal.

An argument could be made that we should never call terminate within a library. But in a way, that is what we are currently doing. Other options are to not throw (bad idea), or do cleanup outside of the destructor i.e. using a public method (an even worse idea IMO).

Another place where a destructor throws is in database_fixture.cpp. This is testing code so not very critical. My suggestion here would be to log the exception, and call BOOST_FAIL to fail the test.

Cleaning these up will help us see other warnings that are sometimes ignored due to these warnings filling our terminals.

Additional ideas are extremely welcomed.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2019

If the undo_db session destructor throws the internal database is left in an undefined state. There is no reasonable way to clean up continue from there. To avoid the immediate exit it could be an option to somehow invalidate the underlying database.

Using BOOST_FAIL in database_fixture sounds good.

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

it could be an option to somehow invalidate the underlying database.

Hmmm. The only advantage I see is that the program keeps running, which may not be a good thing. The database is in an unknown state, unless we can accurately determine the cause and successfully remedy the situation. In that case, I would question the need to throw in the first place. But perhaps you have situations in mind that would benefit from such logic. Would you please elaborate?

@jmjatlanta jmjatlanta removed this from New -Awaiting Core Team Evaluation in Project Backlog Jan 25, 2019

@jmjatlanta jmjatlanta added this to To do in Feature Release (201902) via automation Jan 25, 2019

@jmjatlanta jmjatlanta self-assigned this Jan 25, 2019

@jmjatlanta jmjatlanta moved this from To do to Done in Feature Release (201902) Jan 25, 2019

@abitmore

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

@jmjatlanta this has been done? Why not close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.