Success
Referenced as Pull Request for: # 6470

User tests: Successful: Unsuccessful:

avatar okonomiyaki3000
okonomiyaki3000
5 Aug 2013

Sometimes, if you're on a slow connection (or some other reason) you can timeout while downloading an update package. So, we should reset the time limit on each iteration of the download loop so that it's not so easy to timeout.

avatar okonomiyaki3000 okonomiyaki3000 - open - 5 Aug 2013
avatar okonomiyaki3000
okonomiyaki3000 - comment - 27 Nov 2013

Really, it's silly to timeout while trying to update. Let's not timeout, OK?

avatar Hackwar
Hackwar - comment - 4 Mar 2014

Hmm, I don't completely agree with you here. With your solution, that process might take "indefinitely". When you reset it each time, the PHP process might run longer, but at least when the browser gives up, the whole process dies anyway, so we shouldn't make this longer than maybe 45 seconds, even if it times out. All in all, I see more benefit in working on smaller downloadsizes (hint distribution with small components wink, wink nudge, nudge) than implementing this.

avatar brianteeman
brianteeman - comment - 4 Mar 2014

I dont see anything wrong with this in principle. Even if you have a tiny filesize the internet connection may be poor

avatar mahagr
mahagr - comment - 4 Mar 2014

Personally I want to use rather short PHP timeouts and let the scripts to set higher timeout if they are expecting task to take longer time. So setting timeout higher fixes the issue on having slow download.

But... I would use fixed timeout for 5 or so seconds -- if you cannot download the next 4kb during that time, there's no reason to keep on waiting for something that will never complete on time.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 4 Mar 2014

It doesn't make much difference whether the timeout is 5 seconds or 30 or 60, it is not about the timing of any single iteration of the loop. The point is to reset periodically while the download is in progress and so it is done once per iteration. The reason to reset it each time to the default value (instead of something arbitrary like 5) is that this will remain the timeout value for the rest of the execution of the program unless changed again.

Sure, we could set it to 5 on each iteration then back to the default value immediately after the loop exits, but why add that extra complication?

As to the other points, I think it's unrealistic to hope that all packages will be small. The Jooma! update itself will rarely be a small package. But package size and connection speed might not even be the only causes of this kind of issue. There are a whole range of possible causes. I believe this patch addresses many of them. If there are others, yet unsolved, we should also work on solutions for them. I'll certainly let you know if I find any.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 18 Apr 2014

Ping.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 26 Jun 2014

In case you don't get what my line note is all about, my point is that we are already doing this kind of thing in this very file (as well as many others). So what's the problem? None, I say.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 2 Jul 2014

So is there still any objection to this? I don't really see how there could be.

avatar Bakual
Bakual - comment - 2 Jul 2014

I don't see a problem with this from looking at the code. The only issue I see is that there is zero tests so far.
Maybe you can add better testing instructions in how to reproduce the problem (eg lower the max execution time) and see if the PR fixes the issue.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 2 Jul 2014

It's highly likely that the problem can't be duplicated with specific Joomla, PHP, or Apache settings. I get timeouts probably because of something about our corporate network. It's an edge case but there could be others experiencing something similar.

avatar brianteeman brianteeman - change - 21 Aug 2014
Status New Pending
avatar nicksavov nicksavov - change - 21 Aug 2014
Labels Removed: ?
avatar zero-24
zero-24 - comment - 6 Feb 2015

Hi @okonomiyaki3000

as this issue is still valid but pull requests to the master branch of this repo are not longer accepted. Can you close this pull request and submit a new one against the staging branch?
If there is no response in a month we will close the PR here.

Thanks for understanding!

avatar vdespa vdespa - change - 14 Mar 2015
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2015-03-14 12:53:03
Closed_By vdespa
avatar joomla-cms-bot
joomla-cms-bot - comment - 14 Mar 2015

Set to "closed" on behalf of @vdespa by The JTracker Application at issues.joomla.org/joomla-cms/1668

avatar joomla-cms-bot joomla-cms-bot - close - 14 Mar 2015
avatar joomla-cms-bot joomla-cms-bot - close - 14 Mar 2015
avatar vdespa
vdespa - comment - 14 Mar 2015

@okonomiyaki3000 - Thanks for your work on this issue. As @zero-24 mentioned, this cannot be merged against the master branch. Please revise the issue and submit the PR again. Sorry for the inconvenience.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/1668.

Add a Comment

Login with GitHub to post a comment