Fix temp file leak in pull-through metadata streaming. - #7933
Conversation
| await response.write(data) | ||
| if remote.policy != Remote.STREAMED: | ||
| if save_artifact and remote.policy != Remote.STREAMED: | ||
| await original_handle_data(data) |
There was a problem hiding this comment.
This is where the downloader saves the file to disk?
At the very least this change looks reasonable to me.
There was a problem hiding this comment.
It looks like there are several places where we check for exactly save_artifact and remote.policy != Remote.STREAMED can we redefine the variable so that it already contains the streamed condition?
(Somehow this may be a copy-and-paste / missed-all-the-places-to-update bug coming from exactly there.)
There was a problem hiding this comment.
This is where the downloader saves the file to disk?
Yes, original_handle_data that usually points to BasedDownloader handle_data that does the writing (if not overriden by plugins downloader)
bde34c6 to
2ab7b43
Compare
2ab7b43 to
1011edc
Compare
1011edc to
ef5032d
Compare
| if remote.policy == Remote.STREAMED: | ||
| save_artifact = False |
There was a problem hiding this comment.
Now these lines suprise me again.
I'd rather remove the default True from the function definition and move this comparison to the call site.
ef5032d to
0906b5b
Compare
0906b5b to
6a0f684
Compare
3 participants
Currently there is a problem with non content type files staying indefinitely on the disk when using _stream_remote_artifact() even if save_artifact flag is set to false.
I don't see a good reason for writing this data on disk in the first place, this pr fixes that.
fixes: #7846
📜 Checklist
See: Pull Request Walkthrough