gh-92452: Avoid race in initialization of sysconfig._CONFIG_VARS #92453
Conversation
|
All commit authors signed the Contributor License Agreement. |
ec53780 to
755f474
Compare
|
@hroncok Would you be able to review this? Asking you since you recently worked on sysconfig.py. |
|
I could have a look, but I have no merge rights here. Also, this does not seem to touch the code I touched at all. |
|
I more or less agree with @carlbordum. |
d07fc71 to
afaad34
Compare
afaad34 to
17556a5
Compare
There was a problem hiding this comment.
Thank you @gareth-rees! This looks good, I just have a minor comment.
Signed-off-by: Filipe Laíns <lains@riseup.net>
There was a problem hiding this comment.
I have applied the suggested changes.
The patch looks good to me, the only worry here is that we are introducing a new import, which is a setback for #98718, but I am working on an implementation for #88142 that may not need to pull sysconfig at startup. If this does become an issue for the future plans, we can address it then, but for now it's best to merge this fix.
Thanks @gareth-rees!
| # don't re-enter init_config_vars(). | ||
| if _CONFIG_VARS is None: | ||
| _init_config_vars() |
There was a problem hiding this comment.
The name of the function was changed to _init_config_vars so the comment on line 715 needs to be updated to match.
There was a problem hiding this comment.
This is minor, I think everyone will understand what the comment is saying, but feel free to open a PR updating it.
There was a problem hiding this comment.
I will keep that line in mind for replies to comments on future pull requests.
You're welcome. See above for a comment on FFY00's suggestion. I guess this needs to be applied directly to the main branch now. |
Fixes #92452.
I couldn't figure out how to add a regression test, because the race only occurs while
sysconfig._CONFIG_VARSis still being initialized by the first thread to callsysconfig.get_config_vars, and I don't see how to ensure that my test case (if I added one) is guaranteed to run first. I did check that the reproducer from the issue no longer asserts.