get_contents won't unlink tempfile when ftp_fget returns false

[ruud@…]

Hi,

on line 104 of class-wp-filesystem-ftpext.php the get_contents.php functions bails out after an failed ftp-fget attempt.

However because wp_tempnam() creates and touches a fresh new file, more and more files get onto the server.

The unlink call at line 114 isn't used anymore

[ruud@…]

Crude stab at a patch of some sort

[dd32]

@ruud@joyo Good catch!

The patch looks correct to me too :)

[ruud@…]

Hi dd32,

Thanks for your quick follow-up.
I was looking into this piece of code because our server was bogged down with trying to seek for a new non existing temp file name.
We ran into errors like:
[23-Nov-2015 07:32:00 UTC] PHP Warning: unlink(/tmp/debug180541.tmp): No such file or directory in ..

but that meant the server already had gone form debug1.tmp all the way up until debug180541.tmp opening files.. etc. to get to a non-existing .tmp file.

While I do like enumeration like this, wouldn't something like using a (part of?) unix timestamp also suffice, and be way more faster?

Such an approach would also alleviate possible future unlink problems as well, but I'm not sure if the current methodology of getting unique filenames has more to it, like it has to be sequential or something.

Thanks,
Ruud

[ruud@…]

come to think of it: using timestamps does/may get you into weird stuff when changing server time settings etc. But we could prevent that kind of stuff to do a second round of checks for file existance with the current way, just enumerating to get to a unique filename on top of a timestamp based filename.

[dd32]

In 4.4+ wp_tempnam() includes some random data per-call into the filename, which should greatly reduce the chances of it hitting a conflicting filename.

Theoretically servers shouldn't be hitting this code branch too often, if it is, there's likely something else wrong with the server which needs to be investigated.. but that being said, this patch is definitely good, the only reason I didn't commit it immediately was we were about to go RC1 for 4.4.

[dd32]

In 35777:

Upgrader: FTP: Cleanup temporary files during FTP download failures.
Props ruud@joyo
Fixes #34772

[ruud@…]

Request removed. I noticed in a later install that more contributors were added to the list :)

[ocean90]

In 38094:

Filesystem API: Cleanup temporary file when the temporary file couldn't be opened.

Props ruud@joyo.
See #34772.
Fixes #36942, #36943.