Page MenuHomePhabricator

Factor the execution of a test method into its own function to ensure proper cleanup

Authored by amccarth on Oct 15 2015, 2:56 PM.



This solves problems in tearDown of individual test cases, which were especially noticeable on Windows.

When a test throws an exception, the exception retains references to the functions in the stack, which in term keeps their local variables alive. When those locals are SB proxy objects, cleanup doesn't proceed as it should because they keep LLDB objects alive that should have been orphaned.

By moving the running of the test method to a separate function, we ensure that the exception goes out of scope before we attempt tearDown, and the gc.collect() makes sure the orphaned objects are properly destroyed before the framework attempts to delete the target.

Without this fix, a test case that throws might cause the shared module list to retain an open handle to the inferior, even after the target is deleted. This affects the state of subsequent test cases in the same test. This was especially noticeable on Windows in tests like TestTargetAPI, because the open handle prevented make from deleting the inferior after the failed test case and the next test case from re-building the inferior. Thus nearly all tests cases would fail because of a problem in just one.

Diff Detail


Event Timeline

amccarth updated this revision to Diff 37523.Oct 15 2015, 2:56 PM
amccarth retitled this revision from to Factor the execution of a test method into its own function to ensure proper cleanup.
amccarth updated this object.
amccarth added a reviewer: tfiala.
amccarth added a subscriber: lldb-commits.
tfiala accepted this revision.Oct 15 2015, 3:24 PM
tfiala edited edge metadata.

LGTM, Adrian. Thanks!

This revision is now accepted and ready to land.Oct 15 2015, 3:24 PM
zturner accepted this revision.Oct 15 2015, 3:29 PM
zturner added a reviewer: zturner.
zturner added a subscriber: zturner.
zturner added inline comments.
2548–2550 ↗(On Diff #37523)

This problem is more general than just modules. Maybe it could say "so that we can be sure that all test-specific resources have been freed before we attempt to delete the targets."

This revision was automatically updated to reflect the committed changes.
amccarth marked an inline comment as done.

Nice catch on the comment.