timeout for ftp connections as a constant in config.php

[ntm]

In the last, I had have some problems with the automatic plugin upgrade. Every I startet the upgrade I have got an error message told me the I had entered a wrong password. But I was very sure that I had used the right one.

While reading in the wordpress.org forums I discovered that it could have something to do with the file "/wp-admin/includes/class-wp-filesystem-ftpext.php". And after some testing I could narrow the problem down to the timeout for the PHP function ​ftp_connect in line 19 of the file class-wp-filesystem-ftpext.php. There the timeout = 5 and that is insufficiently at my hosting provider. (A friend of mine has a weblog at a server of the same hosting provider and has experienced the same problems.)

I can imagine that this 5 seconds value is for a reason so small. But for some cases it would be great to be able to change that value easily and that is why i have made two little patches for
/wp-admin/includes/class-wp-filesystem-ftpext.php and /wp-admin/includes/file.php whose make it possible to change the value via the config.php file like all the other ftp credentials.

I have no overview of all the WordPress core functions and files, so it might be necessary to make some further changes to this build in.

[dd32]

Theres another ticket floating around in the Filesystem component by Denis relating to timeouts too..

[dd32]

attachment file.php.patch added

Not sure why its being set like that.. No real point

attachment class-wp-filesystem-ftpext.php.patch added

i'd use the define directly there, ie:

if ( defined('TIMEOUT..') ) 
$this->timeout = TIMEOUT...;

It might be worth just hard-coding a longer timeout there though, possibly 10sec? Bad username/password will return instantly anyway..

What value do you set it to for it to work for you?

[Denis-de-Bernardy]

it's #10407, and probably a very similar or the same issue.

[ntm]

Replying to dd32:

It might be worth just hard-coding a longer timeout there though, possibly 10sec? Bad username/password will return instantly anyway..

Yes, but i think that it is easier for the WP users to change or set the value in the well known wp-config.php as in in some file somewhere in a sub folder of a folder which is not wp-content.
In my opinion, another argument for making it possible to set another timeout value via an entry in the wp-config.php is that there could be configurations where the timeout should be higher than 5 or - in my case - 15 seconds. In comments to the ticket #10407 you can read something about 120 or 600 seconds.

What value do you set it to for it to work for you?

For the plugin upgrade it have to set the value to 15 seconds. (But have not tested this with the WP version upgrade.)
10 seconds are not enough.

[dd32]

than 5 or - in my case - 15 seconds. In comments to the ticket #10407 you can read something about 120 or 600 seconds.

Both tickets are a different timeout too, This is for login, and thats for on-going-transfer-timeout i think.

But i'm +1 on the idea of a define for them both though, say FTP_TIMEOUT_LOGIN and FTP_TIMEOUT, with the addition of a higher-than-current timeout by default just for those who the slightly higher timeout works for (Afterall, a higher timeout isnt going to negatively affect most people)

[dd32]

...FTP_CONNECT_TIMEOUT would suit better than FTP_TIMEOUT_LOGIN of course.. since its not a login timeout :P

[ntm]

Replying to dd32:

attachment file.php.patch added

Not sure why its being set like that.. No real point

I have made that because this timeout value is in my opinion part of the ftp credentials, too and all other credentials like hostname and username are saved in the same way.

attachment class-wp-filesystem-ftpext.php.patch added

i'd use the define directly there, ie:

if ( defined('TIMEOUT..') ) 
$this->timeout = TIMEOUT...;

I have oriented myself at the checks of the other values and have added

0 < intval($opt['connect_timeout'])

because on the manual page of ​ftp_set_option is written that the timeout value must be an integer that is greater than 0 [...]. But i have forgotten to use intval in the next line. It should be

$this->timeout = intval($opt['connect_timeout']);

. Or is better to create a new variable and execute intval in every case only once?

Bad username/password will return instantly anyway..

That is a good point. I think that it should be changed, too. But to get a more precise error message it would probably be necessary to take the time during the ftp_login attempt (e.g. with microtime()) and compare it with the timeout limit.
Is that to much?

[...] the addition of a higher-than-current timeout by default just for those who the slightly higher timeout works for [...]

I think that, too. How big should the default value be? 15 seconds? 30 seconds? more? (BTW: the hard coded timeout value for the ssh2 connections is already 15 seconds. ​class-wp-filesystem-ssh2.php line 55)

(Later today, I will post a changed and supplemented patch for class-wp-filesystem-ftpext.php)

[ntm]

I have tested a little bit more for the changed patch and have discovered that 15 seconds for the timeout are not emough (in my case). So I set the default timeout value to 30 seconds in the new patch.
Furthermore i can precise my report: the ftp_login process needs more than 5 seconds. (ftp_connect has needed considerably less than second in every trial.)

Further i have supplemented the login section of the ​class-wp-filesystem-ftpext.php file with a suggestion for a further error message which is more precise if the time between the start and the end of the login process is longer than the timeout value.

[ntm]

changed default timeout, use timeout constant if defined, further errormessage for login timeout

[ntm]

followed now: If defined, set it to that, Else, If POST'd, set it to that, If not, Set it to whatever it previously was (saved details in option) (comment in line 678 of file.php)

[ntm]

I have thought about my last patch file for the ​class-wp-filesystem-ftpext.php file during the last days. Maybe it was a little bit to much to the additional error message in there as part of this ticket. Maybe it is more correct to open a separate ticket for this issue.

Should I open a new ticket for that?

(I have made another patch without it that message but with the described changes for class-wp-filesystem-ftpext.php and file.php.)

Further I'm not sure if my changes (in class-wp-filesystem-ftpext.php) are ok or not.

DD32 has said:

i'd use the define directly there, ie:

if ( defined('TIMEOUT..') ) 
$this->timeout = TIMEOUT...;

Should I create a patch which changes only class-wp-filesystem-ftpext.php like this (but with the constant name FTP_CONNECT_TIMEOUT)?

What should i do?

[dd32]

What should i do?

I'll clean the patch up and integrate some changes i think is needed for #10407 later this arvo

[dd32]

attachment 10522.diff added

• Adds
  • FTP_CONNECT_TIMEOUT - A Timeout for the initial connecting 30s
  • FTP_TIMEOUT
    • Used by SSH/ftpsockets as a per-action timeout 30s
    • Used by ftpext as a per-connection timeout, Set appropriately higher in that class 240s (4min)
• no sanitization is done on the constants, If a user defines them, they need to make sure its set to a value which suits their system. IMO, the defaults i've selected should be more than enough for most people.
• ftpsockets was previously defaulting to 30s for the connect/action timeouts.

...And the patch is -completely- untested, It may even have fatal PHP errors, but syntax looks sane to me.

[ntm]

I have put 10522.diff into a WP 2.8.2 and tested it a little bit and on the first look, there is a problem:
FS_CONNECT_TIMEOUT is not defined in class-wp-filesystem-ftpext.php and FS_TIMEOUT is only defined in this file because of the lines

if ( ! defined('FS_TIMEOUT') )
   define('FS_TIMEOUT', 240);

And if the constant is not defined the constant name is given to ftp_connect or ftp_ssl_connect as a string which causes a PHP error.

(I will probably test more in the next days.)

[dd32]

Ah you're right, I defined the defaults too late.

try the updated patch. (only differs by changes to file.php)

[ntm]

Yes, now FS_CONNECT_TIMEOUT is known in class-wp-filesystem-ftpext.php.

(There is another thing which I don't understand, yet: I have defined FTP_SSL=FALSE in my wp-config.php but a var_dump() during a plugin upgrade test showed $this->optionsssl? = true. I'm going to investigate this tomorrow or on sunday. Maybe it is nothing or some how my mistake.)

[dd32]

I have defined FTP_SSL=FALSE in my wp-config.php but a var_dump() during a plugin upgrade test showed $this->optionsssl? = true.

FTP_SSL doesnt work like that. Define it to force SSL, As you've found out, It doesnt check the contents of the define, only that its present. Feel free to discuss it in another ticket if you feel strongly.

[dd32]

Any chance of commiting this to trunk for testing?

[azaozz]

(In [11823]) Add constants for ftp connections timeouts, props dd32, see #10522

[ryan]

Fixed?

[dd32]

Calling it as fixed.

Re-open(or file a new bug) if the constants do not operate as intended, Or need to be increased for the majority of hosts (However, The current defaults seem to be working well)

[ntm]

Although it is already closed: I have tested all this with trunk-r11863 and all seems to work well on my test systems.
Thank you for your work and help!

[Denis-de-Bernardy]

Replying to dd32:

Calling it as fixed.

Re-open(or file a new bug) if the constants do not operate as intended, Or need to be increased for the majority of hosts (However, The current defaults seem to be working well)

Itching to re-open this one. Some hosts don't allow to override the value. And I frequently get 10 minutes during clean-ups on this server I'm using to test. All hell breaks loose if any amount of cleaning up is needed. (FWIW, it takes about 10 minutes to manually clean up using FTP software.)

It is better, imo, to disconnect then reconnect, and then to set the timeout to a new value.

[Denis-de-Bernardy]

There are issues and typo in the patch that were committed, too.

[Denis-de-Bernardy]

Also worth a note: set_time_limit() should be tweaked accordingly, else the new constants may end up useless.

[dd32]

wp-admin/includes/file.php

That is defined as a per-action timeout, Not as a entire connection timeout.

wp-admin/includes/class-wp-filesystem-ftpsockets.php

The timeout in that file is on a per-action basis, Setting the FS_TIMEOUT value that high in that class is only ensuring that actions will only time out after a long time.

wp-admin/includes/class-wp-filesystem-ftpext.php

Typo is a valid bug, And increasing it to 300 might solve some peoples problems, Comment changes are not - They're defaults for the class, and its a per-ftpext thing, not a ftp thing.

A better solution is to send a NOOP or similar keep-alive during extracion every 15s or so, I've got some code around here which will work on 2.9+, I'll just have to dig it up.

[Denis-de-Bernardy]

Replying to dd32:

wp-admin/includes/file.php

That is defined as a per-action timeout, Not as a entire connection timeout.

we probably need an extra define, then, so as to allow plugins to bump it to something very large if needed without affecting direct and SSH connection.

wp-admin/includes/class-wp-filesystem-ftpsockets.php

The timeout in that file is on a per-action basis, Setting the FS_TIMEOUT value that high in that class is only ensuring that actions will only time out after a long time.

wp-admin/includes/class-wp-filesystem-ftpext.php

Typo is a valid bug, And increasing it to 300 might solve some peoples problems, Comment changes are not - They're defaults for the class, and its a per-ftpext thing, not a ftp thing.

Are you 100% on this? I ask, because my FTP software (on my desktop) frequently reconnects during WP uploads. To me, it's an FTP thing, rather than an ftpext thing.

Also, it's worth noting that, on Dreamhost, doing the following shell command can time out:

for site in `ls */wp-config.php`; do site=${site%%/*}; cp -R wordpress/* $site/*; done

You end up losing the SSH connection with enough domains.

A better solution is to send a NOOP or similar keep-alive during extracion every 15s or so, I've got some code around here which will work on 2.9+, I'll just have to dig it up.

That would totally rule, if we could keep the bloody thing alive. note, however, that some servers force the maximum time out and disallow to override it. I can't recall which host I saw this occurring on, but I do remember telling the customer to change host precisely because of this. Disconnect + reconnect was the only reliable workaround, but I couldn't achieve that in some areas (namely unzip_file()) due to the lack of plugin hooks.