Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-14051

download_file_content $timeout option is only a connect timeout in url mode

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9
    • Fix Version/s: 1.9.1
    • Component/s: Libraries
    • Labels:
      None
    • Database:
      Any
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE

      Description

      download_file_content has a $timeout parameter. When in emulation (Snoopy) mode, this is the 'read timeout', but in Curl mode it only sets CURLOPT_CONNECTTIMEOUT, the connection timeout, and not CURLOPT_TIMEOUT, the hard timeout.

      I have made a patch. Some notes about it:

      1) I made the current timeout parameter control the read timeout. This makes Snoopy/curl behaviour consistent (changing curl to do what snoopy did).

      2) I changed the default to 5 minutes (300) in case people are using it to download largeish files.

      3) I added an extra 'connect timeout' which defaults to 20.

      The reason for making the normal, first timeout parameter the read timeout one is that you probably won't need to change the connect timeout unless you want to make things really short e.g. link checker, (ii) current code probably expects the timeout parameter to work, which it won't in many cases at present.

      Connect timeout doesn't do anything if using a proxy (well unless your proxy is down), this is documented. [Another reason for focusing on 'real' timeout.]

      I'd suggest fixing this in 1.9x if possible.

        Gliffy Diagrams

          Activity

          Hide
          quen Sam Marshall added a comment -

          To note, I have tested this patch (with a basic script that times how long it takes to exit after retrieving a url that doesn't work) and it appears to work i.e. it times out at roughly the expected time.

          I forgot to note above that I also made the connect timeout work (as a connect timeout) in snoopy (by setting an otherwise-unused private variable) although I haven't been able to actually test this.

          If somebody gives me permission to put it into 1.9 (or just head if you like) I'll do so.

          Show
          quen Sam Marshall added a comment - To note, I have tested this patch (with a basic script that times how long it takes to exit after retrieving a url that doesn't work) and it appears to work i.e. it times out at roughly the expected time. I forgot to note above that I also made the connect timeout work (as a connect timeout) in snoopy (by setting an otherwise-unused private variable) although I haven't been able to actually test this. If somebody gives me permission to put it into 1.9 (or just head if you like) I'll do so.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          My +1. Seems ok. Thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - My +1. Seems ok. Thanks!
          Hide
          skodak Petr Skoda added a comment -

          my +1

          Show
          skodak Petr Skoda added a comment - my +1
          Hide
          quen Sam Marshall added a comment -

          Thanks, I have now committed this to HEAD and MOODLE_19 (tagged as merged).

          Show
          quen Sam Marshall added a comment - Thanks, I have now committed this to HEAD and MOODLE_19 (tagged as merged).
          Hide
          skodak Petr Skoda added a comment -

          verified, closing

          Show
          skodak Petr Skoda added a comment - verified, closing

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                15/May/08