User tests: Successful: Unsuccessful:
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.
Really, it's silly to timeout while trying to update. Let's not timeout, OK?
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.
I dont see anything wrong with this in principle. Even if you have a tiny filesize the internet connection may be poor
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.
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.
Ping.
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.
So is there still any objection to this? I don't really see how there could be.
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.
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.
Status | New | ⇒ | Pending |
Labels |
Removed:
?
|
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!
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-03-14 12:53:03 |
Closed_By | ⇒ | vdespa |
Set to "closed" on behalf of @vdespa by The JTracker Application at issues.joomla.org/joomla-cms/1668
@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.
Tracker: http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=31807&start=0