TEST/COMMON: Fix file descriptor leaks between tests#11222
TEST/COMMON: Fix file descriptor leaks between tests#11222guy-ealey-morag wants to merge 8 commits intoopenucx:masterfrom
Conversation
guy-ealey-morag
force-pushed
the
fix-fd-leaks
branch
from
e3fe880 to
230ce83
Compare
| m_state = RUNNING; | ||
| } catch (test_skip_exception& e) { | ||
| } catch (test_skip_exception &e) { | ||
| cleanup(); |
There was a problem hiding this comment.
we don't want to call cleanup if init() failed (same as we don't call d'tor if c'tor throws)
There was a problem hiding this comment.
In some cases the test_skip_exception is thrown after some initialization already happened, so the cleanup is needed to avoid fd leaks.
I think that the cleanup functions should assume that initialization is not complete and adapt to that (clean only what is initialized, and not assume anything).
It is also possible to check if (m_state != INITIALIZING) to do only part of the cleanup if needed.
There was a problem hiding this comment.
are we able to fix the cases to always throw before init?
There was a problem hiding this comment.
I'll try to map out when it can happen
There was a problem hiding this comment.
@tvegas1 It looks like there are many cases where the exception is thrown during initialization in a way that can't be be performed during check_skip_test() without a major refactor (or possibly not at all)
There was a problem hiding this comment.
ok, "cleanup functions should assume that initialization is not complete and adapt to that " is also valid option so let's document it in the functions' declaration. I'd prefer to avoid "m_state != INITIALIZING" checks, like in test_log for example, and make the instead make the cleanup function more "robust". like done in test_p2p_am for example.
|
Reverted the changes to |
| EXPECT_LE(files_count, ucs_global_opts.log_file_rotate + 1); | ||
| EXPECT_NE(0, files_count); | ||
| ucs_log_init(); | ||
| if (m_state != INITIALIZING) { |
There was a problem hiding this comment.
should we also skip it for NEW?
There was a problem hiding this comment.
cleanup should be never called when m_state is NEW, I also added an assert in ucs::test::cleanup to enforce that
guy-ealey-morag
force-pushed
the
fix-fd-leaks
branch
from
320e172 to
fe43fb1
Compare
|
The major fd leak fix was merged with another PR, and the rest of the fixes proved to be false-positives because the leak check was placed in |
4 participants
What?
Fix file descriptor leaks between tests.