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

[HttpFoundation] Session handlers should use a lock #4976

Open
vicb opened this Issue Jul 19, 2012 · 26 comments

Comments

Projects
None yet
@vicb
Copy link
Contributor

vicb commented Jul 19, 2012

The following session handlers:

  • MemcachedSessionHandler
  • MemcacheSessionHandler
  • MongoDbSessionHandler
  • PdoSessionHandler done

do not use a lock to serialize access to the session data. This might result in a data corruption / lose of data when two requests are modifying the session data at the same time.

see #4668

edit:

This is also applicable to Symfony/Bridge/Doctrine/HttpFoundation/DbalSessionHandler.php

@schmittjoh

This comment has been minimized.

Copy link
Contributor

schmittjoh commented Jul 19, 2012

How about adding a BagInterface implementation which persists data to the backend as soon as it is set instead of waiting on the save call. Likewise we would need to have the get calls directly read data from the backend.

@vicb

This comment has been minimized.

Copy link
Contributor Author

vicb commented Jul 19, 2012

But then the session data might be changed during the lifetime of a request (by an other concurrent request). I don't think this is a desirable behavior.

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 19, 2012

@schmittjoh - that would sort of break the lifecycle of the session since you'd be persisting the session before PHP asked for it. All the native c code have various locking strategies - copying the strategy from the C code would probably work best, although I find the locking strategy of the memcached driver suspicious (using a further memcached call to store the locks) - but if it's working in practice, then it must be ok.

@schmittjoh

This comment has been minimized.

Copy link
Contributor

schmittjoh commented Jul 19, 2012

btw, my idea came from database systems which only lock that part of the data that is necessary to allow for higher concurrency. If we want to do really perfect, we can add a locking mechanism to the session interface so that the developer can decide which information must be locked (read/write) on a per-key basis.

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 19, 2012

@schmittjoh - what do you mean by "part of the data"? The session is a single record.

@schmittjoh

This comment has been minimized.

Copy link
Contributor

schmittjoh commented Jul 20, 2012

It is a single record with the current implementation because we load a
key-value map into the bag at the beginning and store in back in the end,
but why does it have to be like this? We can leave this detail to the
storage implementation and delegate each ->get() call to the storage. If
the storage wants to load the entire session data, it can. If the storage
only wants to load the requested key, it can as well.

If we want to draw the analogy to databases, we are locking the entire
table right now. Instead we could lock only a row (think specific key).
Some keys might never be locked because they are mostly read-only. For
example, if you load some variables into the session when someone logs in,
but then never again change them. For these, we do not need to block all
other requests. That's why I think we could benefit from a more
sophisticated implementation than lock all or nothing.

On Thu, Jul 19, 2012 at 11:16 PM, Drak <
reply@reply.github.com

wrote:

@schmittjoh - what do you mean by "part of the data"? The session is a
single record.


Reply to this email directly or view it on GitHub:
#4976 (comment)

@yanoosh

This comment has been minimized.

Copy link
Contributor

yanoosh commented Jul 20, 2012

@schmittjoh idea is good. When we want to just verify user permission and send static data we do not need to block session for other request.

@frosas

This comment has been minimized.

Copy link
Contributor

frosas commented Jul 26, 2012

Some thoughts on it:

  • @schmittjoh by key locking looks interesting but wouldn't it block in even the simplest cases? For example, when a user is logged in every request would read its authentication data thus blocking other requests. Also, I'm not sure whether locking by key would bring performance issues.
  • Not locking handlers change the default behaviour considerably with both read and write race conditions. If the solution to these race conditions implies using the session in a special way I still would, for coherence and simplicity, lock the whole session data by default.
  • I'm arriving to the conclusion session data is evil, specially the PHP way to handle it which is too simple for current needs. I'm thinking about not using $_SESSION and its lifecycle at all, instead every feature requiring storing data attached to a session can implement it as desired. For example, an implementation for flash messages could store these in a database table along with the session id. Blocking would only occur in the serialized transaction reading/clearing the existing messages and the one writing it.
@pmurzakov

This comment has been minimized.

Copy link

pmurzakov commented Sep 17, 2013

is there any news on this issue?

@mevdschee

This comment has been minimized.

Copy link

mevdschee commented Nov 9, 2013

I implemented session locking in SncRedisBundle and LswMemcacheBundle (ported from pecl memcached), see:

Maybe this is of any help to anyone.

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Apr 11, 2014

Locking here means we need to make sure that the same session cannot be read while another connection already read this session id. So it must block rows between read() and write().

Locking in MySQL could use SELECT FOR UPDATE as described in http://www.mysqlperformanceblog.com/2007/03/27/php-sessions-files-vs-database-based/ but AFAIK non-existent rows are not locked this way. So we would need to INSERT IGNORE in read() or use SELECT GET_LOCK.

In sqlite one can use BEGIN IMMEDIATE with a RESERVED lock. But since sqlite does not support row locking but locks the whole database it would mean that only one session id can be accessed at a time. It prevents parallel processing of sessions.

A more sophisticated storage implementation as suggestion by @schmittjoh is interesting. But reading/saving each key would collide with the SessionHandler interface. You can only get the whole session serialized and not individual keys. So this would be something completely different.

@Tobion Tobion referenced this issue Apr 11, 2014

Merged

Session locking #120

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Apr 14, 2014

The php internal sqlite session handler did not seem to use locking: https://github.com/php/php-src/blob/PHP-5.3/ext/sqlite/sess_sqlite.c I could not find this for PHP >= 5.4. Was this feature removed?

For Posgres this could be used as reference: http://svn.php.net/viewvc/pecl/session_pgsql/trunk/session_pgsql.c?revision=326806&view=markup

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Apr 22, 2014

Since sqlite only supports database locking and no row locking, using locking in sqlite would mean that only one session can be read -> write at a time. Even different sessions would wait for another to finish, i.e. no parallel requests at all. So using sqlite for sessions is highly questionable. Either you have race conditions or you have no parallel requests.
Locking in sqlite could be implemented using "begin immediate transaction" in read() and commiting in write() or close().

See also https://bugs.php.net/bug.php?id=53713

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Apr 23, 2014

For SQLite, I would still use locking (for correctness) and add a note in the phpdoc about why SQLite can only be an option for development or prototypes, not for production.

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented May 15, 2014

Please have a look at #10908. Would be good if someone can test it, esp. Oracle or SQL Server.

fabpot added a commit that referenced this issue May 17, 2014

bug #10908 [HttpFoundation] implement session locking for PDO (Tobion)
This PR was merged into the 2.3 branch.

Discussion
----------

[HttpFoundation] implement session locking for PDO

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #4976 for PDO
| License       | MIT

This is probably the first Session Handler for databases that actually works with locking. I've seen many implementations of session handlers (mostly only for one database vendor) while researching and none used locking. Not even the [PHPs SQLite session handler](https://github.com/php/php-src/blob/PHP-5.3/ext/sqlite/sess_sqlite.c) or [PECL Postgres Handler](http://svn.php.net/viewvc/pecl/session_pgsql/trunk/session_pgsql.c?revision=326806&view=markup) implemented locking  correctly which is probably the reason why they have been discontinued. [Zend Session](https://github.com/zendframework/zf2/blob/master/library/Zend/Session/SaveHandler/DbTableGateway.php) seems not to use locking either. But it saves the lifetime together with the session which seems like a good idea because you could have different lifetimes for different sessions.

- Implements session locking for MySQL, Postgres, Oracle, SQL Server and SQLite.
Only tested it for MySQL. So would be good if someone can confirm it works as intended on the other databases as well.
- Also removed the custom RuntimeException which is not useful and a PDOException extends RuntimeException anyway, so no BC break.
- I added a default for the table name to be in line with the DoctrineSessionHandler.
- Check session.gc_maxlifetime in read(). Imagine we have only ever one user on an app. If maxlifetime is not checked in read, his session would never expire! What I don't get is why PHP calls gc() after read() instead of calling it before... Strange decision. For this reason I also had to do the following to improve performance.
- I delay gc() to close() so that it is executed outside the transactional and blocking read-write process. This way, pruning expired sessions does not block them from being started while the current session is used.
- Fixed time update for Oracle and SQL Server.

Commits
-------

50ec828 [HttpFoundation] implement session locking for PDO
@jbbrunsveld

This comment has been minimized.

Copy link

jbbrunsveld commented May 19, 2014

In my case, locking the session gives problems in a websocket context (Ratchet, io thread), where a lock never seems to be released. A second call to the same session waits untill the previous one has been unlocked. As this never happens, the second call finally ends in an exception.

@andrerom

This comment has been minimized.

Copy link
Contributor

andrerom commented Oct 2, 2014

For memcacheD, as it seems to natively support locking in it's session handler, would it make sense to bring back NativeMemcachedSessionHandler?

(was removed by @Drak in b2cc580 )

update: not needed, can just configure symfony to use php handler and configure memcache there.

@DennisBirkholz

This comment has been minimized.

Copy link

DennisBirkholz commented Jan 3, 2015

I just want to warn against the idea to access keys stored in the session individually as proposed by @schmittjoh. This will introduce a very easy to hit race condition if the storage handler will have locking (which it should if it works properly). You have to be very careful in which order to access session variables and do so in all your code. That will be very hard to debug.

@mevdschee

This comment has been minimized.

Copy link

mevdschee commented Jan 3, 2015

@baptistedonaux

This comment has been minimized.

Copy link

baptistedonaux commented Feb 5, 2015

What the issue's state ?

I use PdoSessionHandler with MySQL and I have errors that I don't arrive to reproduce. I run on Symfony 2.6.3.

[request] Uncaught PHP Exception PDOException: "SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction" at /home/www/vendor/symfony/symfony/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php line 336

I can see that PdoSessionHandler has been fixed (in first post, task finish).

@mevdschee

This comment has been minimized.

Copy link

mevdschee commented Feb 9, 2015

@andrerom Sorry for the late reply, but I think it makes total sense to bring back the NativeSessionHandler classes.

@mmucklo

This comment has been minimized.

Copy link
Contributor

mmucklo commented Mar 4, 2016

In certain contexts it makes sense to allow for parallel requests (think API).

In these cases locking the entire bag/session will block other requests.

I think the suggestion made by @schmittjoh to have "row"-level session locking is the right way to go.

What should happen is the ability to do a get with a read and a get with a read+lock.

Example:

   public function get($key, $lock = false) {

   }

-- delete, update, and get then should all block on a key that's locked, but normal read-only cases should be able to happen in parallel.

@mevdschee

This comment has been minimized.

Copy link

mevdschee commented Mar 4, 2016

@mmucklo: You say that you want to "allow for parallel requests (think API)". This is certainly recognized by PHP and PHP7 now supports read-only sessions with the "read_and_close" option (see: http://php.net/manual/en/function.session-start.php). This allows for parallel requests as long as you don’t write to the session (which is typically the case when an API call is made).

NB: The "session_start" with read-only flag operates on the entire session bag (not on specific keys as @schmittjoh suggests) and that makes it a much easier to avoid deadlock situations.

IMHO, if you are creating an API then you might consider not to use "session_start" at all. By doing so you create a "stateless API". For authentication you may support JWT tokens in the "Autentication" (Bearer) header in which session variables (such as the authenticated user and it's role) are claimed. These claims can be trusted, because they are signed by the authentication provider. Read about this on http://jwt.io/

@mvrhov

This comment has been minimized.

Copy link
Contributor

mvrhov commented Mar 7, 2016

You can't do uploads from javascript with custom headers.

@mmucklo

This comment has been minimized.

Copy link
Contributor

mmucklo commented Mar 7, 2016

Headers may work - @mvrhov what about XMLHTTPRequest.setRequestHeader?

However if we're talking about BFF cases, it seems more like the actual session is what we want as it can be easily tied to the browser.

In that case I'm thinking key-level locking could be advantageous...

Then again if you allow that, what's next, transactions? Transactions truly might then be needed if you then want to update multiple keys together but fail if only one is updated...

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Mar 22, 2017

#21093 does not fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.