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

Added more caching for download streams #37

Merged
merged 3 commits into from
Dec 4, 2017

Conversation

asgrim
Copy link
Member

@asgrim asgrim commented Dec 3, 2017

#36 - enables caching as an experiment to see how it shapes traffic with CloudFlare caching.

* @var array
*/
private $metadata;

/**
* @param RateLimiter $rateLimiter
* @param array $fileList
* @param string $buildDirectory
Copy link
Member

Choose a reason for hiding this comment

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

Please update the docblock before merge.

@jaydiablo
Copy link

I assume that with these changes caching servers (like CloudFlare) will better cache the file downloads, but you risk losing download stats for cache hits.

I think this is a good approach to see if it affects bandwidth at all, but if it does, we may want to explore doing a two step redirect for the file download. The first step (the URL exposed on browscap.org that is used by all auto-updating clients) would collect the stats (and do any of the rate limiting/ip banning that you do now) and then redirect to a second URL that would download the file. The first hit would send headers so that it isn't cached, the second would do the headers that you're doing in this PR, in hopes to trigger aggressive caching by Cloudflare (or any other cache servers between your server and the client).

Unless of course you don't care much about the download stats (or have a different way to retrieve them from CloudFlare).

@asgrim
Copy link
Member Author

asgrim commented Dec 4, 2017

Yep, I think this is a good first step. Download stats are a "nice to have" really, I've got to prioritise the bandwidth consumption reduction to avoid having disruption to people's scripts and things 👍 I'll tweak that docblock and merge this - thanks folks!

@asgrim asgrim merged commit b22b37b into master Dec 4, 2017
@asgrim asgrim deleted the enable-client-side-caching-of-streams branch December 4, 2017 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants