Moodle
  1. Moodle
  2. MDL-36538

Webdav repository runs out of memory with large files

    Details

    • Testing Instructions:
      Hide
      1. Set up a webdav repository
      2. Add a large file (e.g. > 50Mb) to the webdav folder
      3. Make sure the file limits for your test course are greater than the size of the large file
      4. Add a new file resource to a course
      5. On the settings page, click 'Add' on the filemanager
      6. Attempt to add the large file to the filemanager via the webdav repository

      Expected result - the large file can be added successfully.

      Show
      Set up a webdav repository Add a large file (e.g. > 50Mb) to the webdav folder Make sure the file limits for your test course are greater than the size of the large file Add a new file resource to a course On the settings page, click 'Add' on the filemanager Attempt to add the large file to the filemanager via the webdav repository Expected result - the large file can be added successfully.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-36538_webdav_memory_24
    • Pull Master Branch:
      MDL-36538_webdav_memory
    • Rank:
      46008

      Description

      As the webdav repository downloads the entire file into memory, before writing it out to a local file, large files can cause 'out of memory' errors.

      I will submit a patch, in a moment, that writes each downloaded chunk directly to a local file, removing the large file limit.

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks for working on that, Davo.

        Hopefully we can get this peer reviewed soon.

        Show
        Michael de Raadt added a comment - Thanks for working on that, Davo. Hopefully we can get this peer reviewed soon.
        Hide
        Davo Smith added a comment -

        Fred - do you think you would be able to take a look at this sometime soon, so I can put it forward for integration?

        Show
        Davo Smith added a comment - Fred - do you think you would be able to take a look at this sometime soon, so I can put it forward for integration?
        Hide
        Frédéric Massart added a comment -

        Hi Davo,

        sorry for the late review, but I have been really busy with the 2.4 release. A bit like everyone I guess . Here are my comments about your patch:

        • In general, I feel like the PHPDocs are not consistent, sometimes the $params are not described, sometimes there is use of "-" to separate the variable name from the definition, sometimes not. On line 302, the doc is quite hard to read and understand, if the type is bool or string, I'd removed mixed and just use "bool|string". This is no big deal, but helps everyone's understanding of the methods.
        • Speaking about PHPDocs, could you add @since information in new methods? I don't think this will be backported so @since 2.5 is probably what we want here.
        • On lines 1570-1571, could you confirm that we have to use strlen($buffer) and not strlen($chunk)?

        To polish a bit things:

        • In the new function get_to_file(), there is quite a lot of duplicated code from get(), would it be possible to merge those lines into one function?
        • We now have 3 functions, get(), get_file() and get_to_file(), this becomes hard to understand what does what and how. I think you could make use of get_file() instead of creating a new function. We actually don't need to create the file object in repository/webdav/lib.php so perhaps get_file() could do that and implement your new logic.

        Thanks for your work on this issue, let me know what you think of those comments and also your point of view about backporting this.

        Cheers,
        Fred

        Show
        Frédéric Massart added a comment - Hi Davo, sorry for the late review, but I have been really busy with the 2.4 release. A bit like everyone I guess . Here are my comments about your patch: In general, I feel like the PHPDocs are not consistent, sometimes the $params are not described, sometimes there is use of "-" to separate the variable name from the definition, sometimes not. On line 302, the doc is quite hard to read and understand, if the type is bool or string, I'd removed mixed and just use "bool|string". This is no big deal, but helps everyone's understanding of the methods. Speaking about PHPDocs, could you add @since information in new methods? I don't think this will be backported so @since 2.5 is probably what we want here. On lines 1570-1571, could you confirm that we have to use strlen($buffer) and not strlen($chunk)? To polish a bit things: In the new function get_to_file(), there is quite a lot of duplicated code from get(), would it be possible to merge those lines into one function? We now have 3 functions, get(), get_file() and get_to_file(), this becomes hard to understand what does what and how. I think you could make use of get_file() instead of creating a new function. We actually don't need to create the file object in repository/webdav/lib.php so perhaps get_file() could do that and implement your new logic. Thanks for your work on this issue, let me know what you think of those comments and also your point of view about backporting this. Cheers, Fred
        Hide
        Davo Smith added a comment -

        OK, I'll fix the phpdocs and address the issues listed.

        Are you sure this won't be backported? It isn't really a new feature, it is a fix for a fairly major bug in an existing feature (having your server run out of memory when transferring a file that is comfortably within the upload limit, would seem like a major bug, especially if you are using the webdav repository to enable videos to be easily inserted into a course and are hitting the limit a lot).

        Show
        Davo Smith added a comment - OK, I'll fix the phpdocs and address the issues listed. Are you sure this won't be backported? It isn't really a new feature, it is a fix for a fairly major bug in an existing feature (having your server run out of memory when transferring a file that is comfortably within the upload limit, would seem like a major bug, especially if you are using the webdav repository to enable videos to be easily inserted into a course and are hitting the limit a lot).
        Hide
        Davo Smith added a comment -

        I think I've covered the changes - if you're happy with this, I'll also produce 2.2 / 2.3 / 2.4 versions of it, as I think it should be fixed in all versions.

        Show
        Davo Smith added a comment - I think I've covered the changes - if you're happy with this, I'll also produce 2.2 / 2.3 / 2.4 versions of it, as I think it should be fixed in all versions.
        Hide
        Frédéric Massart added a comment -

        Hi Davo,

        yes I think this should be fixed in all version as well, but when it comes to introducing new functions, not sure the integrators really like it. Although I like your patch the way it is, and I think it should be backported.

        Minor notes before your backport and push for integrations:

        • Line 405, before the patch there was use of 'w' or 'wb' when opening the file. Now you're using 'w'. I'm happy with either, what is your opinion on that?
        • Line 407, it appears that $buffer has not been set.
        • Line 1453, the PHPDoc could probably specify that the $fp argument is a file handle, not a file.
        • And if you feel like it, @since 2.2.7, 2.3.4, 2.4.1 could be added to the new static method.

        Thanks for your work on this!

        Cheers,
        Fred

        Show
        Frédéric Massart added a comment - Hi Davo, yes I think this should be fixed in all version as well, but when it comes to introducing new functions, not sure the integrators really like it. Although I like your patch the way it is, and I think it should be backported. Minor notes before your backport and push for integrations: Line 405, before the patch there was use of 'w' or 'wb' when opening the file. Now you're using 'w'. I'm happy with either, what is your opinion on that? Line 407, it appears that $buffer has not been set. Line 1453, the PHPDoc could probably specify that the $fp argument is a file handle, not a file. And if you feel like it, @since 2.2.7, 2.3.4, 2.4.1 could be added to the new static method. Thanks for your work on this! Cheers, Fred
        Hide
        Davo Smith added a comment -
        • line 405 - 'wb' is probably safer, so I've switched to that
        • line 407 - that is how it was in the original code, $buffer is not used within the 'get' function, as the existence of the $fp variable overrides it. Even if $buffer was used, it would be initialised within the 'get' function (as it is a return value, rather than an input value). However, just for completeness, I've renamed it to 'unused' and initialised it.
        • line 1453 - I've updated the PHP doc
        • @since - I've not done this, as the function in question is a private, internal implementation detail, so it isn't part of an external interface that a developer would have to avoid using in earlier versions of Moodle.
        Show
        Davo Smith added a comment - line 405 - 'wb' is probably safer, so I've switched to that line 407 - that is how it was in the original code, $buffer is not used within the 'get' function, as the existence of the $fp variable overrides it. Even if $buffer was used, it would be initialised within the 'get' function (as it is a return value, rather than an input value). However, just for completeness, I've renamed it to 'unused' and initialised it. line 1453 - I've updated the PHP doc @since - I've not done this, as the function in question is a private, internal implementation detail, so it isn't part of an external interface that a developer would have to avoid using in earlier versions of Moodle.
        Hide
        Dan Poltawski added a comment -

        > but when it comes to introducing new functions, not sure the integrators really like it.

        The key point is about being as cautious as we can for a reason. If we avoid big refactors then we reduce the change of introducing new bugs and avoid changing APIs which customisations could be dependent upon. In this case I don't think we are falling too foul of it on either point.

        Show
        Dan Poltawski added a comment - > but when it comes to introducing new functions, not sure the integrators really like it. The key point is about being as cautious as we can for a reason. If we avoid big refactors then we reduce the change of introducing new bugs and avoid changing APIs which customisations could be dependent upon. In this case I don't think we are falling too foul of it on either point.
        Hide
        Dan Poltawski added a comment -

        (Well, actually I suppose this is a fairly big refactor, but I think its worth it on judgement).

        Show
        Dan Poltawski added a comment - (Well, actually I suppose this is a fairly big refactor, but I think its worth it on judgement).
        Hide
        Dan Poltawski added a comment -

        I've integrated this now, thanks guys!

        Show
        Dan Poltawski added a comment - I've integrated this now, thanks guys!
        Hide
        Ankit Agarwal added a comment -

        Works as described.
        Passing!
        Thanks

        Show
        Ankit Agarwal added a comment - Works as described. Passing! Thanks
        Hide
        Ankit Agarwal added a comment -

        Sorry forgot to click the passed button..

        Show
        Ankit Agarwal added a comment - Sorry forgot to click the passed button..
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: