Moodle
  1. Moodle
  2. MDL-14051

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

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      37106

      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.

        Activity

        Hide
        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
        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
        Eloy Lafuente (stronk7) added a comment -

        My +1. Seems ok. Thanks!

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

        my +1

        Show
        Petr Škoda added a comment - my +1
        Hide
        Sam Marshall added a comment -

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

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

        verified, closing

        Show
        Petr Škoda added a comment - verified, closing

          People

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

            Dates

            • Created:
              Updated:
              Resolved: