Repository navigation
Roll back a rejected XML load as a whole, and keep Tree's destructor from throwing - #1289
Open
Quchaosheng wants to merge 2 commits into
Open
Quchaosheng wants to merge 2 commits into
Quchaosheng wants to merge 2 commits into
Conversation
BehaviorTree#1266 dropped a document that failed to load, but the rollback stopped at the document boundary. If main.xml includes sub.xml and then fails, sub.xml has already registered its trees and they stay behind, and sub.xml becomes the first document, so instantiateTree() reads main_tree_to_execute from it. Take a snapshot before the call and restore it on failure. That also puts back a tree this call redefined, whose element lives in the document about to be erased. Fixes BehaviorTree#1282
Tree::~Tree() calls haltTree(), and halting may throw: a TryCatch with catch_on_halt="true" ticks its catch child, and a tick is allowed to throw. An exception leaving a destructor calls std::terminate, so destroying such a tree aborted the process. Mark the destructor noexcept and swallow the exception. There is no caller left to report to. Fixes BehaviorTree#1281
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two fixes for #1282 and #1281.
#1282 - #1266 dropped a document that failed to load, but the rollback
stopped at the document boundary. If
main.xmlincludessub.xmland thenfails,
sub.xmlhas already registered its trees and they stay behind;sub.xmlalso becomes the first document, soinstantiateTree()readsmain_tree_to_executefrom it. Take a snapshot oftree_roots/tree_sources/suffix_countbefore the call and restore it onfailure. That also puts back a tree the call redefined, whose element lives in
the document about to be erased.
#1281 -
Tree::~Tree()callshaltTree(), and halting may throw: aTryCatchwithcatch_on_halt="true"ticks its catch child, and a tick isallowed to throw. An exception leaving a destructor calls
std::terminate, sodestroying such a tree aborted the process. Mark the destructor
noexceptandswallow the exception.
Both come with tests that fail on the current master:
XMLParserTest.RejectedIncludeKeepsNoTreeandRejectedInlineIncludesKeepNoTreesee the included tree survive a rejected load.TreeDestructorTest.HaltThatThrowsDoesNotTerminateaborts withterminate called after throwing an instance of 'BT::NodeExecutionError'.Full suite: 684 passed, 2 skipped. The XML side is also clean under
-fsanitize=address.The
noexceptonTree::~Tree()changes a public header. If you would ratherkeep the signature alone, dropping the keyword and keeping the try/catch in the
body is enough - say so and I will change it.