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

test.support.os_helper.EnvironmentVarGuard is not thread safe #132113

Open
Wulian233 opened this issue Apr 5, 2025 · 4 comments
Open

test.support.os_helper.EnvironmentVarGuard is not thread safe #132113

Wulian233 opened this issue Apr 5, 2025 · 4 comments
Labels
stdlib Python modules in the Lib dir tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@Wulian233
Copy link
Contributor

Wulian233 commented Apr 5, 2025

Bug report

Bug description:

I ran it multiple times with the following code and got different results

Image

import os
import threading
import time
from test.support.os_helper import EnvironmentVarGuard

def worker1(guard):
    for i in range(1000):
        guard['MY_VAR'] = f'value1'
        time.sleep(0.0001)  # Small delay to increase chance of thread switching

def worker2(guard):
    for i in range(1000):
        guard['MY_VAR'] = f'value2'
        time.sleep(0.0001)

guard = EnvironmentVarGuard()
t1 = threading.Thread(target=worker1, args=(guard,))
t2 = threading.Thread(target=worker2, args=(guard,))

t1.start()
t2.start()
t1.join()
t2.join()

final_value = os.getenv('MY_VAR')
print(f"Final value: {final_value}")

This issue I found in #131870, adding a lock to the test_zoneinfo.py, but the original implementation didn't.

BTW, 131870 PR has not been reviewed by anyone yet (more a week)

CPython versions tested on:

3.14, 3.13

Operating systems tested on:

Windows

Linked PRs

@Wulian233 Wulian233 added the type-bug An unexpected behavior, bug, or error label Apr 5, 2025
@picnixz picnixz added tests Tests in the Lib/test dir stdlib Python modules in the Lib dir labels Apr 5, 2025
@picnixz
Copy link
Member

picnixz commented Apr 5, 2025

BTW, 131870 PR has not been reviewed by anyone yet (more a week)

Before reviewing this PR, we first need to make EnvironmentVarGuard thread-safe then.

@Wulian233
Copy link
Contributor Author

Yes, but I'm not good at this. I hope someone else can make it thread-safe :)

@srinivasreddy
Copy link
Contributor

I am trying to understand more about use case. Even if we make thread safe, that does not mean t2 will always finish last, no?
If we really care about ordering , we must enforce in code like this,

t1.start()
t1.join()
t2.start()
t2.join()

@serhiy-storchaka
Copy link
Member

Context managers that change the global state cannot be thread safe.

For example, let we have two threads with two guards:

  • First guard saves old value and sets a new value.
  • Second guard saves old value (set by the first guard) and sets its own new value.
  • First guard restores the saves original value.
  • Second guard restores the value which was set by the first guard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants