Skip to content

gh-132113: Make EnvironmentVarGuard thread safe #132128

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

Closed

Conversation

srinivasreddy
Copy link
Contributor

@srinivasreddy srinivasreddy commented Apr 5, 2025

@brianschubert
Copy link
Member

brianschubert commented Apr 5, 2025

Looks related to #132113 - can you add the issue number to the title?

edit: re "Please do not review yet", you can mark this PR as a draft by clicking the "Convert to draft" button under "Reviewers" in the right-hand column

@srinivasreddy srinivasreddy changed the title Make EnvironmentVarGuard thread safe gh-132113: Make EnvironmentVarGuard thread safe Apr 5, 2025
@srinivasreddy srinivasreddy marked this pull request as draft April 5, 2025 14:39
@srinivasreddy srinivasreddy force-pushed the thread_safe_env_var_guard branch from fbe6202 to a8a295f Compare April 5, 2025 18:43
@srinivasreddy srinivasreddy marked this pull request as ready for review April 5, 2025 18:46
@Wulian233
Copy link
Contributor

Could you add a news entry? thanks!

@python-cla-bot
Copy link

python-cla-bot bot commented Apr 6, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@picnixz
Copy link
Member

picnixz commented Apr 7, 2025

Can you add a test please? and/or confirm that the reproducer given on the issue is fixed?

@@ -794,6 +797,27 @@ def test_linked_to_musl(self):
for v in linked:
self.assertIsInstance(v, int)

def test_threadsafe_environmentvarguard(self):
Copy link
Member

@picnixz picnixz Apr 7, 2025

Choose a reason for hiding this comment

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

This test should be executed more than once. I think, so I would suggest doing something like that (when I executed the script in my terminal, I needed more than one script execution to see the issue)

def test_threadsafe_environmentvarguard(self):
    for _ in range(100):
        self._test_threadsafe_environmentvarguard()

def _test_threadsafe_environmentvarguard(self):
    ...

@picnixz
Copy link
Member

picnixz commented Apr 18, 2025

Following Serhiy's explanation and considering I actually didn't think enough about this, I closed the issue.

@picnixz picnixz closed this Apr 18, 2025
@srinivasreddy srinivasreddy deleted the thread_safe_env_var_guard branch April 18, 2025 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants