-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Fix a race condition in TexManager.make_dvi. #30426
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?
Conversation
lib/matplotlib/texmanager.py
Outdated
@@ -296,12 +298,12 @@ def make_dvi(cls, tex, fontsize): | |||
# will be blocked when `openin_any = p` in texmf.cnf). | |||
cwd = Path(dvifile).parent |
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.
I don't think we need this any more?
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.
I think there is an unused local, but otherwise good to merge.
It's still used by the TemporaryDirectory call. |
lib/matplotlib/texmanager.py
Outdated
# Also generate the tex source in the main cache directory, but | ||
# only for backcompat. | ||
cls.make_tex(tex, fontsize) |
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.
Maybe inline make_tex
for clarity
# Also generate the tex source in the main cache directory, but | |
# only for backcompat. | |
cls.make_tex(tex, fontsize) | |
tex_source = cls._get_tex_source(tex, fontsize), encoding='utf-8') | |
# Also generate the .tex file in the main cache directory, but | |
# only for backcompat. | |
Path(cls.get_basefile(tex, fontsize) + ".tex").write_text(tex_source) |
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.
I went for another approach, which is to move the tex file from the temporary directory to the main cache when done with it as well. How does that look to you?
Path(tmpdir, "file.tex").write_text( | ||
cls._get_tex_source(tex, fontsize), encoding='utf-8') |
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.
And then here just
Path(tmpdir, "file.tex").write_text( | |
cls._get_tex_source(tex, fontsize), encoding='utf-8') | |
Path(tmpdir, "file.tex").write_text(tex_source) |
Previously, a race condition could occur if, while a process had called make_tex (generating the tex file in the global cache) and was going to call the latex subprocess (to generate the dvi file), another process also called make_tex for the same tex string and started rewriting the tex source. In that case, the latex subprocess could see a partially written (invalid) tex source. Fix that by generating the tex source in a process-private temporary directory, where the latex process is already going to run anyways. (This is cheap compared to the latex subprocess invocation.) Apply a similar strategy for make_png as well.
I chose to also fix make_png here as well. |
Previously, a race condition could occur if, while a process had called make_tex (generating the tex file in the global cache) and was going to call the latex subprocess (to generate the dvi file), another process also called make_tex for the same tex string and started rewriting the tex source. In that case, the latex subprocess could see a partially written (invalid) tex source.
Fix that by generating the tex source in a process-private temporary directory, where the latex process is already going to run anyways. (This is cheap compared to the latex subprocess invocation.)
See #30420 (comment) (point (2)).
Edit: did the same to make_png.
PR summary
PR checklist