Content-range support for file_transfer()

• Feature requests go in head, not 4.7.
• The code is broken: $r-1024;.
• The code does not conform to Drupal code style.
• The parsing of the HTTP header is messy and could be simplified using a simple regular expression (int verification, proper syntax, and more).
• As per HTTP 1.1, the proper response in case the range extends beyond the size of the file is "416 Requested Range Not Satisfiable", instead of just adjusting it silently.
• Overall, the coding style is messy and needs to be cleaned up. Macros such as min/max() can be used to make it more readable.

http://www.faqs.org/rfcs/rfc2616.html

Thanks! This solved the problem I was having with windows media player: http://drupal.org/node/102167

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.

Here's an up-to-date patch for Drupal 4.7.

And one for HEAD. I set it to "needs review" for testing, not so that the previous criticisms could be repeated.

File was missing.

Cleaned up code and replaced header() with drupal_set_header().

Still trying to meet the vague "messiness" objection.

Completely rewritten patch to implement the full HTTP spec.

How should this be tested?

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.

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.

Missed an error.

Nice! Coding style looks good and functionality seems to work on FF win/OSX and Opera Win/OSX.

This is RTBC.

Adding support for a basic HTTP feature is a simple change that can be done now.

Hm, I looked through http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.35 which says that:

the server SHOULD return a response with a status of 206 (Partial Content) containing the satisfiable ranges of the entity-body.

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?

You caught a bug. Updated patch attached.

Patch wasn't created from root. Reroll.

I'd call it ready.

This is a new feature and needs to wait for Drupal 7. Thanks.

Patch still applies to HEAD, but with offset. Here's a reroll.

looking at the RFC i'm still not sure this is correct: http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.16

When an HTTP message includes the content of multiple ranges (for example, a response to a request for multiple non-overlapping ranges), these are transmitted as a multipart message. The multipart media type used for this purpose is "multipart/byteranges" as defined in appendix 19.2.

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.

marked #49868: Download Resume support. as a duplicate

Set Last-Modified and ETag headers based on file modification time to support download pause/resume.

The patch #26 cannot apply against CVS HEAD.

Keeping up with HEAD.

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

Test failure : #335122: Test clean HEAD after every commit

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.

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.

subscribe

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.

+++ 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:

    // 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.

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?

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.

Subscribed

Confirming that #34 works as expected for my Drupal 6.20 site.

Has anyone made a D7 version of this(#34)? I tried the sendfile one, but doesn't seem to work...just want somthing likle this except for D7...

Thank You Drupal Community!!

We were having issues with D7 private videos playing in chrome and safari. redhatmatt and I had an fierce collaborative race to get this module ported to D7.

The module could use some cleaning up and fine tuning, but it is working for us as is.

It's not perfect but instead of sitting on it until it is, or we forget about it, we are returning it back into the wild.

thanks,
SY.

Note that we can (and should!) test the correctness of our implementation using REDbot.

Also note that the implementation I'm using in the CDN module has been proven to work: http://drupalcode.org/project/cdn.git/blob/0f19fca6c4c382cdd751ac97346c0....
Source for that code: http://www.thomthom.net/blog/2007/09/php-resumable-download-server/, now https://bitbucket.org/thomthom/php-resumable-download-server

It's been a very long time since the beginning of this request , i created a project and committed the modules to let more people use this handy feature till it gets into core.
http://drupal.org/project/resumable_download

Thanks everyone

Vote for resumeable download. For a system, that shall get called state of the art, it is a must have.

Missing resume function is also causing issues in most browsers when playing back HTML5 videos stored in private filesystem (no timeline in FF and IE, no autogenerated preview picture in IE, noch playback at all in Safari iOS).

As I've read, HTML5 stuff is one of the big new features of drupal 8.

One more, in my eyes also must have, is a modification of the module "resumeable_download" I've offered here: http://drupal.org/node/1927198

It adds support for returning "304 not modified".

Especially as one focus of drupal 8 is optimizing for mobile devices, reduzing useless traffic (cellular traffic too), should be one important point. Returning 304 is one of them.

Please ignore the file beolow this post as I've worked on the 304 code some time, to get it working much better. The final code is now embedded in the last archive of the thread linked in this post.

145 // Conditional GET requests (i.e. with If-Modified-Since header).
146 if (isset($_SERVER['HTTP_IF_MODIFIED_SINCE'])) {
147 // All files served by this function are designed to expire in the far
148 // future. Hence we can simply always tell the client the requested file
149 // was not modified.
150 header("HTTP/1.1 304 Not Modified");
151 }

That code is not that correct as it should be. The IF_MODIFIED_SINCE is to check for modifications of the file on the server since last request. And you cannot tell for shure, that files will not get exchange by another (I do it for example). So it is not correct to general return 304, when a browser requests this check, because a modified file will never get delivered this way. Also this value really has not much to do with the expiration you have explained in the comment. When there is a cache between user and server, that handles this expiration, the request will never reach the server.

Please take a look at my 304 code here: http://drupal.org/node/1927198

This part contains variables and functions, that are part of the module:

// firts we need to know: wants the browser to check it or not?
if(isset($_SERVER['HTTP_IF_MODIFIED_SINCE'])) {

    // read timestamp of the file in browser cache
    $ifs = strtotime($_SERVER['HTTP_IF_MODIFIED_SINCE']);
      
       // get timestamp of saved file
      $modified = filemtime($filepath);  

      //compare
      if($modified == $ifs) {

                // check etag when set only (not in all cases I've testetd, the etag got sent in the request)
                // when you read it, without check before, you produce an entry in drupal log
		if(isset($_SERVER['HTTP_IF_NONE_MATCH'])) {

                        // If timestamps are equal and etag got sent, we take the etag too
			$ifsn = ($_SERVER['HTTP_IF_NONE_MATCH']);
			$filesizeb = filesize($filepath);
			$eetag = (dechex(fileinode($filepath)) . '-' . dechex($modified) . '-' . dechex($filesizeb));

			// compare etag sent by browser with etag generated with the file
			if($ifsn == $eetag) {
				header('HTTP/1.1 304 Not Modified');                        
				resumable_download_drupal_add_http_header('Last-Modified', gmdate('D, d M Y H:i:s', $modified) . ' GMT');
				resumable_download_drupal_add_http_header('ETag', dechex(fileinode($filepath)) . '-' . dechex($modified) . '-' . dechex($filesizeb));
	
				drupal_exit();
			}
		}

                // when there is no etag, timestamp must be enough
		else{
				$filesizeb = filesize($filepath);
				header('HTTP/1.1 304 Not Modified');                        
				resumable_download_drupal_add_http_header('Last-Modified', gmdate('D, d M Y H:i:s', $modified) . ' GMT');
				resumable_download_drupal_add_http_header('ETag', dechex(fileinode($filepath)) . '-' . dechex($modified) . '-' . dechex($filesizeb));
	
				drupal_exit();
		}
	}

The expiration thing is for caches like proxies (when used, I do not), that tells, how long they can cache the file, before they request it from the server again.

115 header("Expires: Tue, 20 Jan 2037 04:20:42 GMT");

Here should be used, what is setup in admin/config/development/performance, and not a static value. Not everybody wants their files in proxycaches till end of all days and as long as this setting does exist, it should not get ignored.

122 header("Last-Modified: Wed, 20 Jan 1988 04:20:42 GMT");

This is wrong too. There should be the timestamp of the last filechange get sent.

Example:

$modified = filemtime($filepath);                               // get timestamp of saved file
header('Last-Modified', gmdate('D, d M Y H:i:s', $modified) . ' GMT');

file_transfer no longer exists and is replaced by BinaryFileResponse which seems to support Content-Range and partial downloads. #1561362: Change file_transfer() to use BinaryFileResponse

So this feature is definitely not going into D7?

That I also would like to know, as I've big trouble to stream large videos from private file system. The webserver is giong to crash completely (fastest when streaming to my iPhone 4) and I really don't know, is it a drupal or a server issue.

You must use more advanced solution for streaming like that, PHP is not suitable, you can try this module https://drupal.org/project/xsend

That plugin is for drupal 5 and 6. Not 7.

Also it is Apache2 only.

I'm running lighttpd.

The patch for Drupal 7 was rejected because the Drupal 7 menu system makes it possible to override Drupal’s private file handling in a contrib module. I do not know of any project for such a contrib module. I only ported the patch to Drupal 7 because I needed this feature in Drupal 6, so I lost interest.

It's 2020 and I don't know if this will help anyone but #34 fixed my issue with Safari not playing videos stored in private files, in Drupal 7.