Skip to content

Conversation

dbu
Copy link
Contributor

@dbu dbu commented Nov 5, 2016

always send a hash lookup request, even for anonymous users.

the symfony HttpCache implementation allows to configure a hardcoded hash for anonymous users if that is desired, but defaults to do a hash lookup.

alternative to #318

cc @lolautruche @andrerom @bdunogier

@dbu dbu force-pushed the anonymous-user-context-lookup branch 2 times, most recently from 9241135 to df4b4fb Compare November 7, 2016 07:50
@andrerom
Copy link
Contributor

andrerom commented Nov 7, 2016

WDYT @lolautruche @bdunogier ?

@andrerom
Copy link
Contributor

Ok, seems we are +1

But does this make sense from your perspective as well @dbu ?

* **anonymous_hash**: Hash used for anonymous user. This is a performance
optimization to not do a backend request for users that are not logged in.
By default, the header is skipped. If you specify a header, that header is
used.
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit unclear. What’s the header you’re talking about being skipped or used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats the old doc, sorry. it should say "By default, a hash lookup is done. If you specify a hash, no hash lookup for anonymous users occurs and the specified hash is used."

@ddeboer
Copy link
Member

ddeboer commented Nov 14, 2016

Ok, seems we are +1

Does that mean you have spoken with @lolautruche and @bdunogier? 😉

But does this make sense from your perspective as well @dbu ?

I have one argument against this change (which does a hash lookup for requests even if they have no Cookie/Authorization header) and in favour of #318 (which doesn’t): performance. Usually HTTP caching is optimised for the largest group of users. Arguably, that’s the anonymous users. Changing defaults to do a hash lookup (and hit Symfony) for anonymous users will degrade performance.

A couple of solutions:

  1. Follow Cleanup anonymous user context #318 instead.
  2. In the docs, offer both VCL variants.
  3. In the docs, emphasise the importance of caching the anonymous hash request.

@dbu
Copy link
Contributor Author

dbu commented Nov 15, 2016

  1. In the docs, emphasise the importance of caching the anonymous hash
    request.

i will work on 3. if the hash is cached, it should be one additional
request per cache lifetime, which is a very small price to pay for
consistency and avoiding errors.

@lolautruche
Copy link
Contributor

Hi

Sorry but I think I'm missing something here. Why do you want to remove the anonymous hash? I don't see the point... Anonymous hash is here for performance if you don't have any session. Difference will be quite big, even if the hash is cached.

However I understand interrogations people may have about this anonymous-no-session hash, so there's more a need of documentation.
And AFAICT, there was an anonymous hash both for SymfonyCache and Varnish.

@dbu
Copy link
Contributor Author

dbu commented Nov 15, 2016

one important reason is #318 (comment) - if you change the permission set of anonymous, a hardcoded hash will not change and thus the cache could be wrong.

its in general confusing when the hash lookup sometimes does not happen. if we cache the lookup, i don't see the cost. the hash lookup response is shared between all anonymous users, so its done only once.

@lolautruche
Copy link
Contributor

Right, but what will be the cache key? AFAIR for session based requests, it was the cookie header. Is it still the case?
Sorry I didn't look into all that for quite a long time 😊

@dbu
Copy link
Contributor Author

dbu commented Nov 15, 2016

we use whatever the backend tells that the hash depends on. which by default are the headers "Authentication" and "Cookies". you need to use vcl to clean up the cookie and only keep the cookies relevant to the backend when using the context hash (otherwise it also won't work for logged in users).

@lolautruche
Copy link
Contributor

Right, so there should not be any problem.

+1 for me

@dbu
Copy link
Contributor Author

dbu commented Nov 15, 2016

great, thanks @lolautruche - there was a lot of back and forth with this, glad we have a solution that looks ok for everyone.

@ddeboer
Copy link
Member

ddeboer commented Nov 15, 2016

So we just need some small adjustments to the doc, @dbu, and we can merge this and publish 2.0 alpha! 🌟

@dbu dbu force-pushed the anonymous-user-context-lookup branch from df4b4fb to 532da82 Compare November 15, 2016 11:03
@dbu
Copy link
Contributor Author

dbu commented Nov 15, 2016

i just updated the doc. can you check again if that makes sense? then lets get this merge, merge #329 and tag 2.0.0-alpha1 !

@dbu
Copy link
Contributor Author

dbu commented Nov 15, 2016

regarding making it clear that the hash lookup needs to be cached: http://foshttpcache.readthedocs.io/en/latest/user-context.html#caching-the-hash-response is pretty clear to me. anything we could add? or any other place we should point this out?

@ddeboer ddeboer force-pushed the anonymous-user-context-lookup branch from 532da82 to 9098d49 Compare November 15, 2016 12:59
@ddeboer
Copy link
Member

ddeboer commented Nov 15, 2016

Amended the commit to fix the build, hang on...

@ddeboer ddeboer merged commit 9da74bd into master Nov 15, 2016
@ddeboer ddeboer deleted the anonymous-user-context-lookup branch November 15, 2016 14:16
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.

4 participants