Added Active Storage support for byte ranges by tomprats · Pull Request #41437 ·...

Copy link

Contributor

arpu commented on Feb 25

using this branch on my cloudflare -> nginx -> puma -> rails stack and it works fine for ios safari
only problem is cloudflare bypass caching for mp4 per Page Rule worked

arpu commented on Feb 25

response from cloudflare is
The following have to be true:

The origin must be sending Content-Length headers.
The origin must be sending Accept-Range headers.

would it be possible to add the Content-Length headers too?

arpu commented on Feb 26

can confirm this works fine with cloudflare now

arpu commented on Mar 1

@tomprats should i create a new PR for this?

tomprats commented on Mar 1

@tomprats should i create a new PR for this?

Sorry I haven't been able to take another look at this in a couple weeks. I'll try to update it this week

tomprats commented on Mar 2

I've updated the branch to add support a couple more things:

• Multipart ranges - Sends multiple parts separated by headers and a boundary
• Content length - Defaults to full blob's size. When it's a single range, it gives that chunk, when it's a multiple range it gives the total of all the parts sent
• Invalid ranges - When it's only 1 range it's supposed to error to send 416. When there are valid ranges with it, it simply skips the invalid

There are a couple spots I'm not confident on:

• Content length of multipart
• Boundary - I used one fo the examples from the docs but I believe we could be randomly generating it

I won't be able to test this for a little while, but would love some feedback or help. I was planning to rebase main back in once this was finished but let me know if you'd prefer a rebase or merge before then

arpu commented on Apr 9

any Plans to merge this?

tomprats commented on Apr 21

I updated this from scratch due to some restructuring and changes in ActiveStorage (original commits still available in https://github.com/tomprats/rails/tree/active-storage-byte-range-backup).

Some extra changes since last time:

• Updated boundary to be generated by SecureRandom
• Added line breaks to multipart responses
• Added some new test cases for the different types of byte range requests

Some info about the boundary: I used SecureRandom because that's what's used to generate random IDs elsewhere in Rails. RFC says it can be no longer than 70 characters, but it seemed like smaller ones were often used in the examples - https://tools.ietf.org/html/rfc2046

Here are some of the other resources if anyone wants to verify what I've been going off of:

Let me know if there's anything else holding it back! I'm testing it out in a couple of projects

tomprats commented on Apr 22

Learned some new things tonight. Since we're streaming, we don't need the content length (we'd either have to guess at what it might be, or make all the requests first which would negate the benefits of streaming).

I'm not sure if it's not supported by curl but I can't seem to reproduce a successful multipart curl with streaming.

I was able to produce the correct output when switching from send_stream to send_data. Due to byte ranges already being smaller in size, I think it would probably be beneficial to switch to send_data. I will give this another look tomorrow and potentially update the PR with that solution.

tomprats commented on Apr 23 •

I was trying to figure out a potential issue with content-length:

1. Looks like it might be getting removed still due to something magical (does not show up in curl and produces a warning in the server's terminal).
2. The value of the header in the test does not correspond to what I set it in the proxy controller.

send_data might technically still be streaming, but differently than send_stream. Or rack or rails might be doing something I can't figure out. Something like this but then I don't know why it's still in the test.

Here's a warning the server spits out when I curl the proxy. I cannot find the origin of what is producing the warning:

[2021-04-22 23:41:14] WARN Could not determine content-length of response body. Set content-length of the response or set Response#chunked = true

All in all, I think I've done the best I can and maybe these content length issues are meaningless. It appears to work in my app. I think I'm done until I receive some feedback. Thanks for coming to my TED talk.

tomprats commented on Jun 6

Could someone take a look at this? Should be ready

cc @zzak because it looks like you've been reviewing some of the most recent active storage PRs

zzak commented on Jun 7

@santib Since you have already taken a look at this wdyt?

zzak commented on Jun 23

@tomprats Could you rebase -> add a changelog -> and squash please?

tomprats commented on Jun 23

@zzak Should be all set!

arpu commented 24 days ago

any hope to get this in for Rails 7.0 ?

tomprats commented 16 days ago •

Sorry for the wait, I was on vacation for a week. Going to take a look at this tonight or this weekend

tomprats commented 16 days ago

• Updated everything with your feedback @rafaelfranca
• Re-squashed it down into 1 commit again

kaspth commented 5 days ago

@tomprats perfect, thank you!