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

Reduce server load caused by anonymous viewing. #825

Closed
wants to merge 2 commits into from

Conversation

BenLubar
Copy link

Do not start a session if the current user is not logged in for public-facing pages.

Mark pages that don't care about sessions as publicly cacheable.

Keep the max age as 0 so proxies and browsers will still try to retrieve an updated version but can still fall back to the stale version if the site is down or too slow.

Ported from mastodon#9059

Do not start a session if the current user is not logged in for public-facing pages.

Mark pages that don't care about sessions as publicly cacheable.

Keep the max age as 0 so proxies and browsers will still try to retrieve an updated version but can still fall back to the stale version if the site is down or too slow.

Fixes mastodon#9035.
@hannahwhy
Copy link
Member

Is mastodon#9035 resolved? In particular, is the "change page after load using script" behavior, or something like it, implemented?

@BenLubar
Copy link
Author

This circumvents that issue by only telling proxies they can cache pages if no user is logged in.

@hannahwhy
Copy link
Member

hannahwhy commented Nov 23, 2018

Ah, okay, I see now.

Uh, I guess I'd like someone else to look at this, maybe give it a spin. It seems fine to me, but I haven't set up monitoring to see what percentage of requests are to potentially publicly cacheable pages on my own instance. (Well, really, I wonder what the numbers would be like on something more popular like glitch.social.)

If there's no objections after a few days, I'll merge it in. There's at least one instance (https://mastodon.lubar.me) that's already running it.

@ClearlyClaire
Copy link

So the idea is to avoid starting sessions when that's not useful, and enable fallback caching for “public” pages, so that they can be served by a caching proxy should the instance go down.

This looks like a good change, but I'm unsure about skip_session enabling caching by itself: there are more ways to access private information than using a session (e.g., HTTP Signatures).

@hannahwhy
Copy link
Member

What's the word on this PR? I have no preference either way, but the code seems fine and it looks like it sets out what it meant to do.

@ClearlyClaire
Copy link

I'd rather have it modified to have the expires_in calls explicit in the caller instead of being made by skip_session!, as I reported in the equivalent upstream PR.
Otherwise, I'm fine with it, it seems like a good change.

@ClearlyClaire
Copy link

Closing, as it has been merged upstream (and also, does not work properly since skip_session! isn't effective anymore with Rails 5 :()

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

Successfully merging this pull request may close these issues.

3 participants