Posted by dbanon on November 10, 2010 at 12:40pm
10 followers
| Project: | Weather |
| Version: | 6.x-5.16 |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | toddy |
| Status: | closed (fixed) |
Issue Summary
There is a problem fetching data in HTTP mode (weather_fetch = 'HTTP'). It seems the server (weather.noaa.gov) does not close the connection opened with 'fsockopen'.
According to the PHP documentation (first warning in : http://php.net/manual/en/function.feof.php), the 'feof()' function hangs.
A solution would be to modify the loop:
File: weather.module
Function: _weather_retrieve_data($icao)
while (!feof($handle) && !preg_match("/($icao [0-9]{6}Z .+)/", $response, $matches)) {
$response .= fgets($handle, 1024);
}
Comments
#1
The same goes for 6.x branch. Stoppe my frontpage from ever getting rendered. Had to disable the block until we have a better way handling this.
#2
Subscribe for 6.x
#3
this brought down 2 of our sites, we had to disable the module
#4
Subscribe
#5
Weather should not try to update outside of cron. This was causing our site to freeze while it waited for the weather to download. Going to work on a patch that only updates weather on cron. weather_get_metar will try database or internet; should only be database as this can kill your site if the 3rd party server is not playing nicely; would rather have cron die then not serve pages to actual users.
#6
Subscribing. Took down our site as well :(
#7
Now only updates on cron OR if a new weather station was just added. Added a status report entry for the weather module as well on admin/reports/status.
#8
#9
#10
time to go home...
#11
I'm not sure that the cron approach is the correct one. Depending on the cron intervals, you might get seriously outdated and useless weather information (think of a cron run once a day). Therefore, I prefer the module to stay the way it is with regard to downloading new weather data.
Also, the call to feof() described by dbanon is no longer in the 6.x and 7.x versions of the module, so I cannot change that line. It has been changed to call drupal_http_request() in order to allow the module to be run behind a proxy (see #847344: Weather Module, not connecting through a proxy). As I understand, there's no option in D6 to set a timeout on that function, there is however in D7, which I will implement in the module.
So the question is now how to proceed -- there's been a bug open for quite some now for D6 to fix this problem, see #156582: drupal_http_request() should support timeout setting.
Regards,
Tobias
#12
How about an option that disables downloading on page load.
#13
#14
Has this been fixed yet for Drupal 6? It has crashed my website recently and I had to switch to a module which I do not like. I would prefer to use this one.
#15
Hi,
I've created a fix for this problem. In my opinion, the solution is to use a custom copy of drupal_http_request() and modify it so that it supports a timeout parameter. This has now been done, the timeout is 10 seconds. Moreover, the patch introduces an intelligent download scheduler, which is backported from the D7 branch. The module is then able to reduce the download attempts if they are not successful. Instead of every ten minutes, the download interval is 3, 6, 12, and 24 hours; from then on, once a day.
Please test this patch, if there are no objections, I'll release a new version of weather in the next days.
Regards,
Tobias
#16
The last submitted patch, weather-967646-15.patch, failed testing.
#17
+++ weather.module 19 Nov 2010 18:44:03 -0000@@ -2133,3 +2165,228 @@
+function weather_drupal_http_request($url, $timeout = 10, $headers = array(), $method = 'GET', $data = NULL, $retry = 3) {
I would tack timeout on at the end of this function. Follow what is being done for the 6.x patch #156582: drupal_http_request() should support timeout setting
Powered by Dreditor.
Also think about update on cron (configurable via gui) and some useful info on the status page. That's what my patch does; u try it yet?
#18
Is something being worked out for Drupal 5?
#19
Not yet for D5
I'd imagine if this proves successful, there's a chance it will be backported
#20
This bug should be fixed in the recent release of weather, please try it and report any problems you still find.
WRT to a backport for Drupal 5.x: I'm sorry, but I won't backport this for D5. As soon as D7 is released, D5 won't get any security support anymore, so I would urge you to update your site to D6 now. It seems that D7 will be released quite soon.
#21
Automatically closed -- issue fixed for 2 weeks with no activity.