[HttpFoundation] fix PDO session handler under high concurrency

[Tobion]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#8448 and http://trac.symfony-project.org/ticket/4777 (which was never really fixed as you can see here)
License	MIT

• The first commit fixes PDO session handler under high concurrency.
• The second commit uses MERGE SQL for MS SQL Server. Tested with http://sqlfiddle.com/#!6/66b6d/14
• The third commit uses INSERT OR REPLACE for sqlite session handler http://sqlfiddle.com/#!7/e6707/3

What I find rather bad with the class design is that it depends on the table definition, but it's not part of the class. Also it doesn't make use of open() and close() which could be used to make the database connection lazy instead of having is open all the time when not needed. Doctrine also only lazy connects, but we use PDO directly here.
Furthermore, the session handlers should not throw exceptions, from what I read, but return false when an error occurs. This is not followed in this class. Maybe @Drak knows how php session management behaves when the session handlers return false?

[Tobion]

This was one main problem: It creates duplicate keys when session created meanwhile (between select and insert). There is no need to create an entry in read() at all.

[Tobion]

For oracle sysdate was used which is of type DATE. But we expect an INTEGER column, otherwise gc() would not work. So this didn't make sense.

[fabpot]

@Tobion Thank you so much for working on this issue, this is much appreciated. Is it ready to be merged? Or do you also want to implement lazy connections as well in this PR?

[ghost]

@Tobion The PR looks good.

If I remember correctly you should not throw exceptions inside write() or close() because they will never be caught. In general you should return bool where required as it tells the PHP engine (which calls these methods internally) if things worked correctly - no return is also OK since it evaluates to false anyhow. If the methods return false they do not trigger errors. read() evaluates false to an empty string. If a read() fails, it's taken to be a record not found.

Is that helpful?

[Tobion]

@fabpot yes it's ready. Lazy connections would only be for master anyway.

So if we want to make it respect the SessionHandlerInterface we should not throw exceptions but return false. But then I wonder how to best warn developers if something went wrong. For example when a developer forgots to create the table and we just return false everywhere, the dev would have hard times debugging the problem. One solution could be to use a logger in the class and log errors instead of throwing exceptions, or we can use trigger_error() which then depends how the error handler is configured. @Drak what do you think?

[ghost]

Oh gosh sorry I meant to say that in my post but forgot. I was going to suggest adding a logger or raise a PHP userspace error. Logger would be better IMO.

[Tobion]

Warning As of PHP 5.0.5 the write and close handlers are called after object destruction and therefore cannot use objects or throw exceptions.
www.php.net/manual/en/function.session-set-save-handler.php

What does it mean they cannot use objects? PDO and the possible logger are objects as well.

[Tobion]

Ok how I interpret it, is that objects are only destructed before write/close when you didn't call session_write_close() or session_register_shutdown() explicitly but relied on PHP to do it automatically at termination of the script (legacy style). So we should be fine.

[ghost]

@Tobion Yes, this is already handled by the symfony session code, which registers the shutdown handler as session_write_close() automatically

[ruudk]

Just deployed this code to my server and seeing a lot of errors like this:

PHP Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 1205 Lock wait timeout exceeded; try restarting transaction'

Am I the only one?

[Tobion]

Can you give us more information please. Like which database are you using?

[Tobion]

Also since this class only throws RuntimeExceptions, the PDOException you got probably comes from somewhere else. A problem could be that you use the same connection which now interferes. Maybe you leave a transaction open without commiting or rolling back.

[ruudk]

@Tobion Sorry, I am using MySQL and I have a PDO instance only for sessions and use Doctrine2 for other queries (that has a separate connection).

[Tobion]

The PdoSessionHandler throws \RuntimeException(sprintf('PDOException was thrown when trying to write the session data: %s', $e->getMessage()), 0, $e); which is not what you reported. So are you sure the source the the handler?

[ruudk]

Hmm, could be coming from somewhere else then. I've switched to Redis Sessions. Thanks for helping.

[onewheelskyward]

We just ran into this problem with postgres on 2.3.13, it looks like that database might need some attention. Has anyone else encountered this before I start down the path?

[Tobion]

@onewheelskyward what problem did you encounter?

[onewheelskyward]

@Tobion As soon as we updated to 2.3.13, we got the duplicate session id errors. A revert to 2.3.12 fixed it for the time being.

Uncaught exception 'PDOException' with message 'SQLSTATE[23505]: Unique violation: 7 ERROR: duplicate key value violates unique constraint "sessions_pkey"DETAIL: Key (session_id)=(keychangedtoprotecttheinnocent) already exists.' in /path/to/app/code/vendor/symfony/symfony/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php:201

[Tobion]

Hm I don't see how this is possible. Postgres uses a DELETE followed by INSERT inside a transaction. See https://github.com/symfony/symfony/blob/2.3/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php#L189 So the INSERT normally cannot create duplicate key.
Are you sure you are using the above class? Also the exeption that is thrown should be PDOException was thrown when trying to write the session data: ...
Does the problem occur only sometimes or for every session?

[onewheelskyward]

I double checked the config, we are using that class. It seemed to be intermittent but we reverted as soon as we saw the errors. I'll try to recreate on our dev environment with a load test to be sure. Our PDO calls are set as follows, could that be why we got the statement exception instead of the in-function exception handling?

        - [setAttribute, [3, 2]] # \PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION

[tcql]

Just to follow up, I also am seeing the Unique constraint violation with Postgres 9.3.4 with Symfony 2.4+. I'm also pretty puzzled because a single transaction with a delete and then an update should preclude any possibility of this happening.

[onewheelskyward]

Now that's interesting. And I agree with you on the implementation of the
code. Must be something else happening. I haven't had time to go back and
investigate yet, please let me know if you find anything. We're using
postgres 9.3.4 as well.

On Tue, May 13, 2014 at 11:22 AM, tchannel notifications@github.com wrote:

Just to follow up, I also am seeing the Unique constraint violation with
Postgres 9.3.4 with Symfony 2.4+. I'm also pretty puzzled because a single
transaction with a delete and then an update should preclude _any_possibility of this happening.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/10652#issuecomment-42992727
.

[tcql]

I'm poking around, looks like it could have something to do with postgres' default transaction / locking setup, but I'm not positive. Theoretically, if two transactions try doing the same action at the same time, given Postgres' default READ COMMITTED transaction mode

T1	T2
LOCK row for DELETE
DELETE	see row is LOCKed, waiting...
INSERT	waiting...
COMMIT	waiting...
release LOCK
	LOCK the row
	DELETE
	INSERT
	COMMIT

But it seems more like the deletions and insertions of multiple transactions are overlapping somehow

EDIT: yep, related to locking / transaction isolation. running "LOCK table $this->table IN EXCLUSIVE MODE" at the beginning of the transaction fixes things. But a full table lock is very undesirable for concurrency / performance.

[tcql]

Okay, apologies for the annoying multiple posts. Here's what I came up with that fixes it for me:

https://gist.github.com/tchannel/391759d0551639ae0a31

[Tobion]

Yes that's how it should be. Maybe your database is configured with a different transaction isolation level, i.e. Read uncommitted

[tcql]

@Tobion in postgres, READ COMMITTED and READ UNCOMMITTED are the same. In Theory READ COMMITTED should work appropriately, but it doesn't.

[Tobion]

@tchannel can you then open a ticket on postgress or find out why it does not block. Have you tried to execute it manually through postgres command line using two connections? One should block then. Maybe it's a PDO problem.
Using Serializable is not a real solution. We need to use explicit locks anyway (which serializable does not do) to fix #4976
WOuld be good if you find the best way to block a row (existent or non-existent yet) in read() with postgres. For the other DBs I already have a solution in mind.

[tcql]

@Tobion oh very weird. Doing it manually with two psql instances is VERY interesting. If I open both transactions, issue a delete in one, then issue a delete in the second, the second waits for the lock to release (as it should), so I can't enter anything else.

Then, I run the insert in the first transaction, and commit.

Now the second transaction releases because the lock is free. But... the delete count is ZERO. It's like committing the first transaction is somehow doing things as individual commands still... The delete lock is released before the insert is fully processed, and the second transaction runs it's delete before the first transaction's insert goes through.

So definitely not a PDO problem, it's an issue with postgres

[Tobion]

Actually that behavior could be the one described for read commited: http://www.postgresql.org/docs/9.1/static/transaction-iso.html

The delete of the second transaction is just skipped because it didn't match before blocking and the delete is not re-evaluated for the whole table again.

This works where delete is executed after concurred insert, but your example above will indeed create duplicate key.

T1	T2
LOCK row for DELETE
DELETE
INSERT	see row is LOCKed, waiting...
COMMIT	waiting...
release LOCK
	LOCK the row
	DELETE
	INSERT
	COMMIT

[tcql]

eesh. That's... unexpected? I made a bug report / question on the postgres-bugs mailing list. I'll follow up on here when/if I hear anything

[Tobion]

I think it's expected but just highly incomprehensible on first sight.
We need to find a fix, but more in regards to #4976 which comprises this one.

[Tobion]

@onewheelskyward and @tchannel please have a look at #10908. Would be nice if you can test it.

[gheydon]

I have had a problem upgrade past 2.3.13. I get the following excetion thrown.

Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[07002]: [Microsoft][SQL Server Native Client 10.0]COUNT field incorrect or syntax error' in E:\Inetpub\test\api\vendor\symfony\symfony\src\Symfony\Component\HttpFoundation\Session\Storage\Handler\PdoSessionHandler.php:181 Stack trace: #0 E:\Inetpub\test\api\vendor\symfony\symfony\src\Symfony\Component\HttpFoundation\Session\Storage\Handler\PdoSessionHandler.php(181): PDOStatement->execute() #1 E:\Inetpub\test\api\vendor\symfony\symfony\src\Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy.php(77): Symfony\Component\HttpFoundation\Session\Storage\Handler\PdoSessionHandler->write('ifamr16dbjnalto...', '_sf2_attributes...') #2 [internal function]: Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy->write('ifamr16dbjnalto...', '_sf2_attributes...') #3 [internal function]: session_write_close() #4 {main}

The server in question is running sqlsrv 2005.

[Tobion]

@gheydon I think sqlsrv 2005 does not support the MERGE SQL statement used in https://github.com/symfony/symfony/blob/2.3/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php#L236 Seems to be available since 2008.
Can you please give me the output of $this->pdo->getAttribute(\PDO::ATTR_SERVER_VERSION) so I use the fallback for it?

[Tobion]

@tchannel and @gheydon see #11009 for solutions to your problems.

[RuslanZavacky]

Is this going to be merged to 2.4 & 2.5?

[Tobion]

It is already.