-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-120932: Check the pyc file mtime at import #121620
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
base: main
Are you sure you want to change the base?
gh-120932: Check the pyc file mtime at import #121620
Conversation
|
|
The reproducer from #120932 could be turned into a test: import importlib
from pathlib import Path
MODULE_NAME = "tmod"
MODULE_PATH = Path(MODULE_NAME + ".py")
with open(MODULE_PATH, "w") as f:
f.write("a = 1")
mod = importlib.import_module(MODULE_NAME)
old = mod.a
with open(MODULE_PATH, "w") as f:
f.write("a = 2")
mod = importlib.reload(mod)
new = mod.a
assert old != new # failed, 1 != 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, the test fails with about 10% probability.
It seems that overwriting a file not always updates its mtime. I do not know what to do with this.
| except OSError: | ||
| pass | ||
| else: | ||
| if bytecode_mtime < source_mtime: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still possible to get a stale bytecode if write the new source very quickly after creating the pyc file. Using <= instead of < here would be more reliable, but this breaks several existing tests which create bad pyc files very quickly after creating py files.
Using nanosecond precision may make all this more reliable without breaking existing tests, but this will require changing the public interface -- adding the mtime_ns key in path_stats().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using nanoseconds precision does not help.
|
I fixed unstable tests. @brettcannon, what are your thoughts about this? This is not a panacea, you still can import a stale pyc file if rewrite the source file very quickly after creating the pyc file. But this significantly reduces the chance of this. And I tried to minimize the cost. Is it worth to do this? |
|
I think the idea is fine if the performance hit is minimal (it's |
|
Let me think about it and I will get back to you this month (probably at the core dev sprints). |
|
If pyperformance doesn't show a perf hit then I'm fine with this change. We can always revert it later if it turns out to be problematic. |
| @@ -0,0 +1,2 @@ | |||
| Import checks now also the modification time of the ``.pyc`` file for | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Import checks now also the modification time of the ``.pyc`` file for | |
| Import now also checks the modification time of the ``.pyc`` file for |
| bytecode_mtime = 0 | ||
| delay = 1e-3 | ||
| for j in range(10): | ||
| time.sleep(delay) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to set the mtime programmatically or mocking out the appropriate code to avoid the sleep call?
|
I'm fine with the change if pyperformance doesn't show any perf hit. We can always revert if it turns out to be an issue. |
Uh oh!
There was an error while loading. Please reload this page.