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

fix(api): refactor protocol api integration tests to prevent thread leakage #16834

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

jbleon95
Copy link
Contributor

Overview

This fixes an issue found where after a recent mergeback commit into edge, API unit tests were hanging and when escaped, would show as a bunch of OSError: [Errno 24] Too many open files.

Through investigation this was found to be caused by too many integration tests that individually called simulate.get_protocol_api. There is a known issue with this in that the threads this creates (Hardware API and for 2.14 and above contexts, Protocol Engine) are not cleaned up properly. This was not causing any issues in our test suite until recently when the magic number of tests that could be run without too many threads opening was hit.

In order to fix this, the simulating context was refactored to be a pytest fixture. In this new fixture, we yield a ProtocolContext and then afterwards, we close the threads. For engine based contexts, we take advantage of the glboal ExitStack() simulate uses to create the PE context to close and unwind the context stack they are created in, allowing it to be properly garbage collected. For older contexts we just call the clean_up function on the hardware API instance.

Test Plan and Hands on Testing

This is a test only fix, so if CI passes this covers this PR.

Changelog

  • refactor protocol_api_integration tests to prevent thread leakage

Review requests

Can someone else who has this failing test this on their machine to ensure this fix works for not just me.

Risk assessment

Low.

@jbleon95 jbleon95 requested a review from a team as a code owner November 14, 2024 19:47
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Like it or not, this is what peak performance looks like

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks!

@SyntaxColoring
Copy link
Contributor

I was not having the original issue on my machine, but running make -C api test does still work for me with this PR.

@vegano1
Copy link
Contributor

vegano1 commented Nov 14, 2024

Tested this on my machine, LGTM.

@jbleon95 jbleon95 merged commit feeb999 into edge Nov 14, 2024
26 checks passed
@jbleon95 jbleon95 deleted the protocol_api_integration_tests_thread_leakage_fix branch November 14, 2024 20:42
Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

5 participants