Remove the need of temp files from the handling of Range requests

[dratini0]

This commit requires sabre-io/http#45 to work.

The other day, I tried to play a 10 gig HD movie from my Owncloud server through GVFS. It failed, and hanged my server by forcing PHP to create humongous temp files. (see owncloud/core#15321) As it turns out, the problem was with HTTP Range requests: SabreDAV decided to create temp files with the the entire contents of the range (GVFS requested ranges like 40960-). So I went ahead to create this pair of small patches.

I am looking forward to hearing your opinion on the matter. Sorry for not opening a discussion beforehand, but discussing it would have taken longer than writing it.

And yes, with these applied, I have managed to play the video, at least in a separate SabreDAV install.

[Hywan]

One extra space before right.

[Hywan]

Good idea and I like the new solution. More clever and straightforward.

However (I think it is not related to this new patch but it applies in all situations), what if we ask for a range from 42-100 and the stream only contains 7 bytes. Another scenario is if the stream is corrupted. The $consumed variable will never reach the $start variable and we will have an infinite loop. I think we must detect if $consumed stagnates (does not increase) during 2 steps in the loop, or if we reach the end of the stream. The latter solution is more elegant because we can use feof. Nevertheless, remind the stream can be corrupted, the algorithm could therefore never reach the end of the stream and feof is inefficient. Consequently, I propose to add an invariant checking $consumed increases at each step, something like this:

for ($previousConsumed = $consumed = 0; $start - $consumed > 0;) {

    fread($body, min($start - $consumed, $consume_block));
    $consumed = ftell($body);

    if (!($previousConsumed < $consumed)) {
        // break
        throw new Exception(…); // don't know which exception
    }

    $previousConsumed = $consumed;
}

We could add the invariant $previousConsumed < $consumed in the for(…; $invariant;) but we would not be able to throw an exception. And I think it clarifies the code.

[evert]

Love the idea and the quality of the patch. Really great.

[dratini0]

That is nice to hear! 😄