Session locking

[bradleypeabody]

Session locking

[bradleypeabody]

I think what is happening here is that you have a specific use that works for you, and while I'm sure it's a valid one, it is not necessarily the only way to do it. Your note above implies a separate memcache per-machine - but that is not always the case, in fact it is very common to have a shared memcache on a separate server(s). (And in this case the behavior you describe above is not what happens.)

That said, it does make some sense to perform locking in memcache - I can see that working. This should not be the default behavior though. Doing so would mean that calling store.Get(r, "session-name") would issue a lock (effectively blocking any other access to this session until whatever timeout happens), and yet that is not obvious when you are calling a "Get" method. (Also, "Store" is an interface in Gorilla, http://www.gorillatoolkit.org/pkg/sessions#Store , so you can't really just add a GetReadOnly method)

This is not something I'm able to spend time on, but I'd be willing to merge a pull request that has this implemented as an option that could be turned on, not default.

[mevdschee]

Your note above implies a separate memcache per-machine - but that is not always the case, in fact it is very common to have a shared memcache on a separate server(s). (And in this case the behavior you describe above is not what happens.)

In that case the user won't even notice the fail-over as you don't need sticky sessions at all.

Doing so would mean that calling store.Get(r, "session-name") would issue a lock...

But the problem is that you need to know whether or not the intention is to write to the session or not. If you do an AJAX call that requires login you want a non-blocking read-only session, while a normal "page load" may require adjusting the session data and should thus be locking.

The fact that the Gorilla interface does not facilitate expressing this intention merely explains why this feature is missing from all implementations, but it does not make it right to (strictly) follow that interface (I consider it a design flaw in Gorilla). See also: gorilla/sessions#10

PHP has recently (version 7) added this (read only session) feature, that is so important to achieve both high performance and reliability, see: http://php.net/manual/en/function.session-start.php

[mevdschee]

I like the proposal in the gorilla session issue:

store.Lock(r, "session-name")
defer store.UnLock(r, "session-name")
session, _ := store.Get(r, "session-name")
session.Values["foo"] = "bar"
session.Values[42] = 43    
session.Save(r, w)

and for read-only (no locking):

session, _ := store.Get(r, "session-name")
user := session.Values["user"]

I think that is very simple and allows the user to choose.

[bradleypeabody]

I agree, I like it.

[bradleypeabody]

Yes, that's true, but it's also intentional. In an environment with multiple instances of an application, it is not possible to use simple in-memory locking mechanisms to synchronize access (because the instances are on different machines). And a more complex multi-machine locking mechanism more or less defeats the purpose of using a solution as simple as memcache.

But this is usually how memcache-backed applications operate in general. You design the app and the pattern of how it updates memcache with the knowledge that there might be race conditions. It is true that this can be deal-breaker for some applications (I would not be doing payment-related transactions based solely on memcache, for example), but for many it is not.

[mevdschee]

Okay, well I do build high traffic sites and sticky sessions on your load balancer ensures that you hit the right machine. I suggest the following (where 'Get' acquires the lock and 'Save' releases it):

session, _ := store.Get(r, "session-name")
defer session.Save(r, w)
session.Values["foo"] = "bar"
session.Values[42] = 43    

and for read-only (no locking):

session, _ := store.GetReadOnly(r, "session-name")
user := session.Values["user"]

Calling Save would then fail (because the lock was not acquired). I think that such a setup would avoid race conditions.

[bradleypeabody]

Is that a global lock, one for all sessions? Or a lock for each session?

Right now there is no concept of keeping one of anything per session in memory - this allows the overall memcache size to be completely independent from the memory in each machine. So adding a lock for each session would mean adding a big map with a sync.Mutex for every session.

If you mean a global lock, that's not a good idea because it's serializes all session access and in high traffic environments will kill performance.

As a additional note, sticky sessions do not take into account failover (presumably the reason you have load balancing in the first place), so while it's true that this helps, it doesn't guarantee - you can still have race conditions when machines fail (or even just get slow enough for the load balancer to take a machine out of the cluster).

[mevdschee]

You should implement a lock per session. The lock can be kept in memcache using the 'add' operation to ensure atomicity on an extra key (with '.lock' suffix).

I wrote this for PHP, see: https://github.com/LeaseWeb/LswMemcacheBundle/blob/master/Session/Storage/LockingSessionHandler.php#L114

I think it should not be too hard to implement.

[mevdschee]

As for your additional note: sticky sessions may lead to (some) lost sessions on server failure (because the session stickiness is not respected and the other machine's Memcache does not know about the session). This may in practice be acceptable. There are no race conditions, just lost sessions.

[bradleypeabody]

Definitely agreed that the Gorilla interface does not express this difference between read-only Gets and Gets that are followed by a write.

As an idea, maybe explicit methods like GetAndLock and GetNoLock are added to MemcacheStore and then there is an option to indicate which of these is used by Get.

Existing code would run as-is because Get would by default call GetNoLock. With the appropriate option, this could be changed so Get calls GetAndLock, and the caller would have to know what they are doing if they set this option. For more flexibility, the caller could just cast directly to *gsm.MemcacheStore and call whatever methods directly.

[mevdschee]

Okay, I may need a few days to implement (and test) this. Please have some patience...

[bradleypeabody]

Of course :)

[bradleypeabody]

I'm closing this for now.