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

prevent throw in destructor #1511

Merged
merged 2 commits into from
Jan 24, 2019
Merged

prevent throw in destructor #1511

merged 2 commits into from
Jan 24, 2019

Conversation

jmjatlanta
Copy link
Contributor

@jmjatlanta jmjatlanta commented Jan 2, 2019

This is an improvement for Issue #1246.

PR #1510 lowered the number of warnings, but did not eliminate the cause.

I created this PR as an adjunct to #1510 so that we can determine the goods and bads of this approach. I look forward to your comments.

@oxarbitrage
Copy link
Member

I think this is good together with #1510

In order to merge and clean a bit commit history i think we can close this one and #1510 in favor of a new pull with just 3 commits or similar:

  • move session() definition to cpp.
  • prevent throw in destructor.
  • signed/unsigned.

}
return;
} catch (fc::exception& ex) {
BOOST_FAIL( ex.to_detail_string() );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admittedly CAPTURE_AND_RETHROW seems pointless if it doesn't capture anything. Perhaps LOG_AND_RETHROW instead? Or why do you BOOST_FAIL on fc::exception only is better? (Note that fc macros also catch and handle other exceptions.)

@pmconrad
Copy link
Contributor

I wouldn't call this a "true fix" because the end result is still an unclean application exit on library error.

@jmjatlanta
Copy link
Contributor Author

I wouldn't call this a "true fix" ...

I changed it to "an improvement" to be more accurate. A "true fix" may be

  • swallowing the exceptions thrown in the destructor (not a good idea)
  • only calling methods that do not throw exceptions (not possible without the option below)
  • call a "shutdown" method before destruction (error prone)

None of these sound good to me. I am open to suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants