Conversation
|
🧐 There might be something to this. I saw two different flaky failures (only on PyPy, which probably handles fds and cleanup differently to CPython) on #10042, both related to file reading:
|
|
I’m trying to validate it resolves the issue on my other PR as well. If it does then I think this might be a reasonable fix. |
|
Also as you can see I’ve been nerdsnipped during my lunch break. |
| with pytest.raises(ValueError, match="cannot write empty image"): | ||
| im.save(out, compression=compression) | ||
|
|
||
| def test_save_error_cleanup(self, tmp_path: Path) -> None: |
There was a problem hiding this comment.
OK, I can confirm that this test fails 100/100 runs before the fixes in this PR. The output file position is moved during the cleanup. It succeeds 100/100 runs with these changes.
I'm not sure if the bugs in the test suite are due to literally sharing a Python File object or if it was unlucky file descriptor reuse, but either way, I this test should pass, IMO.
There was a problem hiding this comment.
As far as I can tell, it's the latter.
The mechanism seems to be:
- Start a TIFF save into a disk file using the libtiff encoder. Libtiff gets an fd.
- The save raises before finishing, e.g. with "ValueError: cannot write empty image". Before this PR,
TiffImagePlugin._savedidn't dofinally:, so the encoder isn't explicitly cleaned up. - Something keeps the encoder object alive, so its cleanup isn't yet called.
- The output file is closed, freeing its fd number. The libtiff encoder still has that fd though.
- Unrelated code opens another file, and we get the lowest free fd, which unluckily is also known by the stray TIFF encoder.
- Something triggers GC and the encoder is collected and its cleanup is run.
ImagingLibTiffEncodeCleanupcallsTIFFClose/TIFFCleanup, which flush pending output into the now-unrelated file, clobbering it. 😞
Apparently PyPy's GC is different enough (no refcounting, I understand?) that step 6 happens way further off "in other tests" in PyPy land, and with #9945 having landed, the test order is shuffled enough that this happens more obviously...
|
Yep, I found the same reproducer :) The TIFF encoder is probably the only one that flushes data out when it's destroyed... |
| // that is fine, as it does not close the file | ||
| TIFFClose(tiff); | ||
| } | ||
| clientstate->tiff = NULL; |
|
fwiw, I think you commented rather than approved. Not sure if that was intentional |
akx
left a comment
akx
left a comment
There was a problem hiding this comment.
LGTM (from the sidelines).
I'm not sure test_encoder_cleanup needs to test those variations of things that could go awry within the try: finally: block, though. (You could just have the patched _getencoder return a thing that only has a cleanup mock, no setimage(), and see that encode raises and cleanup got called. But that's a matter of taste, I think.)
|
This would be only instance in our test suite of using |
This branch has not been deployed
3 participants
An LLM wrote the code.
It seems that Pillow doesn’t close the encoder even though the file is closed when a failure occurs. If we happen to reuse the same file descriptor number later, I think the encoder can later accidentally corrupt some unrelated file that happened to have the same fd.