Content-range support for file_transfer()
IsoTeemu - October 30, 2006 - 00:48
| Project: | Drupal |
| Version: | 8.x-dev |
| Component: | file system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Darren Oh |
| Status: | needs work |
| Issue tags: | Needs tests |
Description
A patch that adds file transfer resume support by accepting byte ranges and sending 'HTTP/1.1 206 Partial Content' in private file handling.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal-file-range.diff.txt | 1.49 KB | Ignored | None | None |
#1
$r-1024;.http://www.faqs.org/rfcs/rfc2616.html
#2
Thanks for feedback, but dang, comp^Wsuggestion list is bigger than my patch. Taking the hint.
True, I had missed the 416 header part, so I added it. Next I changed it to use preg_match(), but alas, I had been in delusion that regexes should be avoided in simple cases, as they're suppose to be a bit expensive to do. But call me a dumm, as what I can't understand where is that you would like to use min/max. In comparison?
And I think I was a bit rushing, as looking a "correct" way of handling ranges from apache source revealed that there is much more than meets the eye at first sight. So I'll close this and let it wait in my own tree for infinite future.
#3
Thanks! This solved the problem I was having with windows media player: http://drupal.org/node/102167
#4
Too often an imperfect contribution is criticized so strongly that it seems no contribution would have been preferred. Supporting pause and resume for downloads is an important feature. The flaws in this patch should motivate other developers to work on it.
#5
Here's an up-to-date patch for Drupal 4.7.
#6
And one for HEAD. I set it to "needs review" for testing, not so that the previous criticisms could be repeated.
#7
File was missing.
#8
Cleaned up code and replaced header() with drupal_set_header().
#9
Still trying to meet the vague "messiness" objection.
#10
Completely rewritten patch to implement the full HTTP spec.
#11
How should this be tested?
#12
Two ways: with a download manager that supports pause and resume, and by jumping past the downloaded portion of a song in a music player.
#13
RealPlayer works well for testing the seek function. Cancelling and restarting a download in FireFox is a good test of the pause and resume function. Make sure to set downloads to private for a valid test.
After some testing on my own, I modified the patch.
#14
Missed an error.
#15
Nice! Coding style looks good and functionality seems to work on FF win/OSX and Opera Win/OSX.
This is RTBC.
#16
Adding support for a basic HTTP feature is a simple change that can be done now.
#17
Hm, I looked through http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.35 which says that:
Which means to me that if multiple ranges are requested, all ranges should be returned combined. This code does not seem like doing that from what I see. It does move the file pointer to the latest range and sets the length to the latest range requested. Seems to be printing out Content-Range headers for all satisfiable ranges but then only returning the last one. Do I misunderstand the standards or the code?
#18
You caught a bug. Updated patch attached.
#19
Patch wasn't created from root. Reroll.
#20
I'd call it ready.
#21
This is a new feature and needs to wait for Drupal 7. Thanks.
#22
Patch still applies to HEAD, but with offset. Here's a reroll.
#23
looking at the RFC i'm still not sure this is correct: http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.16
it doesn't seem like we'd be returning multipart media.
i'd like to see _file_transfer_range() take a starting offset rather than having to know that you need to fseek() before calling it.
also, the new string concatenation rules aren't followed.
#24
marked #49868: Download Resume support. as a duplicate
#25
#26
Set Last-Modified and ETag headers based on file modification time to support download pause/resume.
#27
The patch #26 cannot apply against CVS HEAD.
#28
#29
Keeping up with HEAD.
#30
this code just seems scary and easy to screw up. i'm of the opinion that we could do this in contrib using the technique i've outlined here: http://drupal.org/node/74472#comment-1052308
#31
The last submitted patch failed testing.
#32
Test failure : #335122: Test clean HEAD after every commit
#33
This does look like something which might be better suited for contrib. Or at least it needs tests - should at least be able to check headers.
#34
I've made module based on last patch of Darren Oh (http://drupal.org/comment/reply/91934#comment-1070336) and advice of drewish (http://drupal.org/comment/reply/91934#comment-1070346). See attachment.
It seems work on my site, though it's not tested in detail yet.
#35
subscribe
#36
I think this would be a great core feature, and would make private transfers more consistent with public file behavior (Apache, at least, has the byteranges filter enabled by default). In addition to resumable downloads, byte range requests are used by the iPhone (and probably other mobile devices) to emulate video streaming over plain HTTP.
For those looking for a contrib solution, the X-Sendfile will also solve this problem.
#37
+++ includes/file.inc 21 Oct 2008 18:07:33 -0000@@ -1233,10 +1233,116 @@ function file_transfer($source, $headers
+ $ranges[$key] = array('from' => max(0, $end_file + $range), 'to' => $end_file);
If I'm reading the RFC correctly, this is off by one. $end_file + $range should use $filesize instead.
Regarding the "scariness" factor, it's quite easy to write tests for this code, and it's not something that will need to change very often (not until the HTTP spec is changed, probably). Here's a sample of what tests would look like:
<?php// The first 500 bytes (byte offsets 0-499, inclusive): bytes=0-499
$this->drupalGet($filepath, array(), array('Range: bytes=0-499'));
$this->assertResponse(206, t('Received HTTP/1.1 206 Partial Content.'));
$headers = $this->drupalGetHeaders();
$this->assertEqual($headers['accept-ranges'], 'bytes', t('Received accept-ranges header.'));
$this->assertEqual($headers['content-length'], '500', t('Content-Length is 500 bytes.'));
$this->assertEqual($headers['content-type'], 'image/jpeg', t('Content-Type is image/jpeg.'));
$this->assertEqual(strlen($this->drupalGetContent()), 500, t('Received 500 bytes.'));
$this->assertTrue(substr($this->drupalGetContent(), 0, 1) === chr(255), t('First byte matched correctly.'));
?>
This review is powered by Dreditor.
#38
I took a look at this module. There seems to be some weird code path though:
if (1) {_file_transfer_range($filepath, $headers);
} else {
file_transfer($filepath, $headers);
}
There is no case in which the else block will get executed. Is the condition right? Or can the control structure be completely removed and just keep what is in the if block?
#39
Either the "1" is supposed to become some variable or setting, or it's debug code and the condition can be safely removed.
Edit: Wait, if the patch enters core, we won't need the contrib module anyway.