Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for multipart/byterange responses #114

Closed
wants to merge 11 commits into from

Conversation

jcready
Copy link
Contributor

@jcready jcready commented Aug 4, 2016

Properly handles byte-range requests with multiple non-overlapping ranges by responding with a multipart/byterange response. For details on the specification for handling this type of request see RFC 7233

For example, a request looking like this:

GET /nums
Host: example.com
Range: bytes=0-1,3-3,5-6,7-8

Would generate a response like this:

HTTP/1.1 206 Partial Content
Accept-Ranges: bytes
ETag: W/"39aa72-153af0d74c0"
Content-Type: multipart/byteranges; boundary=BYTERANGE_IS5K8FWO
Content-Disposition: attachment; filename="nums"
Date: Tue, 02 Aug 2016 18:39:09 GMT
Content-Length: 311
Connection: close

--BYTERANGE_IS5K8FWO
Content-Type: application/octet-stream
Content-Range: bytes 0-1/9

12
--BYTERANGE_IS5K8FWO
Content-Type: application/octet-stream
Content-Range: bytes 3-3/9

4
--BYTERANGE_IS5K8FWO
Content-Type: application/octet-stream
Content-Range: bytes 5-8/9

6789
--BYTERANGE_IS5K8FWO--

stream = fs.createReadStream(path, range)
stream.on('error', next)
stream.on('end', function onpartend () { process.nextTick(next) })
if (!idx) self.emit('stream', stream)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how you want to handle this. I'm creating a new read stream for every range/part so right now I'm only emitting the 'stream' event on the first iteration. I could emit this event on every iteration, but I feel like that might be confusing for a developer using this library since they would end up seeing this event get triggered N times, but the 'end' event would only get emitted once.

// adjust for requested range
offset += ranges[0].start
len = ranges[0].end - ranges[0].start + 1
} else if (ranges.length > 1) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this path should be restricted to some maximum array length. It could be made configurable but would default to say 10.

@jcready
Copy link
Contributor Author

jcready commented Oct 30, 2016

@dougwilson I'd greatly appreciate a review on this :) I promise I won't make any more commits to this branch. I think I finally got everything nailed down with regards to node < 0.10.

@jcready
Copy link
Contributor Author

jcready commented Jan 23, 2017

Closing this because I'm working on something better. Will get a new PR up soon.

@jcready jcready closed this Jan 23, 2017
@dougwilson
Copy link
Contributor

Sweet. I'm sorry I haven't really sat down to review it, but I have definitely been opening the emails when you make commits, which has mainly been on the race condition PR recently.

@dougwilson dougwilson added the pr label Jan 23, 2017
@dougwilson dougwilson self-assigned this Jan 23, 2017
@dougwilson
Copy link
Contributor

I did honestly kind of drop this a bit since I didn't assign it to myself, it wasn't showing up in the GitHub search results I try to use to track everything happening.

@develar
Copy link

develar commented Nov 5, 2017

This feature is still not supported? Doesn't work for me :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants