Skip to content

Fix session leak in PeersV2NodeRefreshIT test #294

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

Merged

Conversation

dimakr
Copy link

@dimakr dimakr commented May 2, 2024

PeersV2NodeRefreshIT#should_successfully_send_peers_v2_node_refresh_query integration test does not close its test session, leading to leaks when integration tests suite is executed in CI. This affects SessionLeakIT#should_warn_when_session_count_exceeds_threshold test, which sees the leaked session and fails due to an unexpected number of active sessions.

The change adds post-test cleanup in PeersV2NodeRefreshIT to ensure that session is properly closed after the test, preventing it from affecting subsequent tests.

@dimakr dimakr marked this pull request as ready for review May 2, 2024 16:25
@dimakr dimakr requested a review from Bouncheck May 2, 2024 16:25
Copy link
Collaborator

@Bouncheck Bouncheck left a comment

Choose a reason for hiding this comment

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

I think it would be better to just put the session creation in a try-with-resources (like try (CqlSession session = ... ) { testMethodBody }. This would match the rest of the tests.

PeersV2NodeRefreshIT#should_successfully_send_peers_v2_node_refresh_query integration
test does not close its test session, leading to leaks when integration
tests suite is executed in CI. This affects
SessionLeakIT#should_warn_when_session_count_exceeds_threshold test, which sees the
leaked session and fails due to an unexpected number of active sessions.

The change wraps the test in try-with block so that the test session is autoclosed at
the end, preventing it from affecting subsequent tests.
@dimakr dimakr force-pushed the PeersV2NodeRefreshIT_session_leak branch from dbab958 to bea9b3a Compare May 6, 2024 09:53
Copy link
Collaborator

@Bouncheck Bouncheck left a comment

Choose a reason for hiding this comment

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

LGTM

@avelanarius avelanarius merged commit 597f0c8 into scylladb:scylla-4.x May 6, 2024
10 checks passed
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.

3 participants