snc/SncRedisBundle

Add a session locking warning and/or recommend Symfony's NativeSessionStorage

mevdschee opened this issue · 2 comments

In the documentation you recommend to use Symfony's Redis session storage handler RedisSessionHandler, but unlike your own session storage handler (that was removed in 4.0) this handler does not support locking. Maybe you want to recommend the NativeSessionStorage and/or add a warning about this regression.

NB: I wrote the session locking implementation in SncRedisBundle, see: #120

This is clearly documented in symfony documentation

The main drawback of this solution is that Redis does not perform session locking, so you can face race conditions when accessing sessions.

I don't think we have to document it on our side as well. What we documented is just an example, there are multiple different solutions as well like you mentioned as well. However, if you have concrete proposal, PR is welcome. Even better, adding the support to Symfony's RedisSessionHandler. That said, I'm sorry if I caused you issues by removing the session handler from this bundle. But I really believe we should centralize the work in Symfony's one.

@ostrolucky Thank you for your reply.

However, if you have concrete proposal, PR is welcome

I did, thank you, see #682

But I really believe we should centralize the work in Symfony's one.

I agree and I'm working on that.

That said, I'm sorry if I caused you issues by removing the session handler from this bundle.

Kind of you, but there is no need to be sorry. I'm well aware of the risks of not locking the session, but I'm afraid that many people aren't. Locking is implemented in mod_files in the session extension not configurable and enabled by default and thus expected by users to be enabled when switching to another module/method.

https://github.com/php/php-src/blob/master/ext/session/mod_files.c#L209