gh-120932: Check the pyc file mtime at import#121620
gh-120932: Check the pyc file mtime at import#121620serhiy-storchaka wants to merge 5 commits intopython:mainfrom
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 |
serhiy-storchaka
left a comment
There was a problem hiding this comment.
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.
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.
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.
| 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.
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.