Skip to content

gh-100227: Port dup3_works to the module state variable. #100262

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

gh-100227: Port dup3_works to the module state variable. #100262

wants to merge 3 commits into from

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Dec 15, 2022 •
edited
Loading

dup3_works is safe to be ported as the module state variable since there is just one writing accessing in L9462.
Even if there are duplicated updatings for the dup3_works due to the race condition, dup3_works will not be updated anymore after the initial trial.

@bedevere-bot bedevere-bot mentioned this pull request Dec 15, 2022
Closed
@netlify
Copy link

netlify bot commented Dec 15, 2022 •
edited
Loading

✅ Deploy Preview for python-cpython-preview canceled.

Name Link
🔨 Latest commit 6ece310
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/639ac64d32320a00083a23a6

@ericsnowcurrently
Copy link
Member

IMHO, we should keep all cached process-level resource availability/capability checks as static global variables. I'm specifically talking about the following group (and any others we might add in the future):

cpython/Tools/c-analyzer/cpython/ignored.tsv

Lines 16 to 23 in b7e4f1d

## indicators for resource availability/capability
# (set during first init)
Python/bootstrap_hash.c py_getrandom getrandom_works -
Python/fileutils.c - _Py_open_cloexec_works -
Python/fileutils.c set_inheritable ioctl_works -
# (set lazily, *after* first init)
# XXX Is this thread-safe?
Modules/posixmodule.c os_dup2_impl dup3_works -

Making any of them per-interpreter is unnecessary since the value will always be uniform for the process. Moving them to _PyRuntimeState did not seem correct because the value is independent of Python's runtime and should not change across init/fini cycles.

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Dec 15, 2022 •
edited
Loading

tl;dr I'd rather we keep dup3_works as a static variable and not move it to the module state.

In the case of dup3_works the only concern I had was if there were any potential races, since the value is set lazily after runtime init. Looking at it again, there really isn't a race there. The only real race would be if two threads both see a -1 value (line 9463, if (dup3_works == -1)) and set the variable. However, that has no consequences if we assume word-size values like this are written atomically, which they should be. So let's not worry about it.

@corona10
Copy link
Member Author

In the case of dup3_works the only concern I had was if there were any potential races, since the value is set lazily after runtime init. Looking at it again, there really isn't a race there

Okay, I understood :)

@corona10 corona10 closed this Dec 16, 2022
@ericsnowcurrently
Copy link
Member

Thanks for working on this though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees
No one assigned
Labels
awaiting core review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@corona10 @ericsnowcurrently @bedevere-bot