[HttpFoundation] enhance PdoSessionHandler #10931

Tobion · 2014-05-17T18:30:58Z

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	#5483, #2067, #2382, #9029
License	MIT

Continuation of locking implementation ([HttpFoundation] implement session locking for PDO #10908): Implement different locking strategies
- PdoSessionHandler::LOCK_TRANSACTIONAL (default): Issues a real row lock but requires a transaction
- PdoSessionHandler::LOCK_ADVISORY: app-level lock, safe as long as only the PdoSessionHandler accesses sessions, advantage is it does not require a transaction (not implemented for oracle or sqlsrv yet)
- PdoSessionHandler::LOCK_NONE: basically what is was before, prone to race conditions, means the last session write wins
Save session data as binary: Encoding session data was definitely the wrong solution. Session data is binary text (esp. when using other session.serialize_handler) that must stay as-is and thus must also be safed in a binary column. Base64 encoding session data just decreses performance and increases storage costs and is semantically wrong because it does not have a character encoding.
That saving null bytes in Posgres won't work on a character column is also documented

First, binary strings specifically allow storing octets of value zero and other "non-printable" octets (usually, octets outside the range 32 to 126). Character strings disallow zero octets, and also disallow any other octet values and sequences of octet values that are invalid according to the database's selected character set encoding.
http://www.postgresql.org/docs/9.1/static/datatype-binary.html#DATATYPE-BINARY-TABLE
Implement lazy connections that are only opened when session is used by either passing a dsn string explicitly or falling back to session.save_path ini setting. Fixes [HttpFoundation] PdoSessionHandler should connect to database only if session required #9029
add a create table method that creates the correct table depending on database vendor. This makes the class self-documenting and standalone useable.
add lifetime column to session table which allows to have different lifetimes for each session
add isSessionExpired() method to be able to distinguish between a new session and one that expired due to inactivity, e.g. to display flash message to user
added upgrade and changelog notes

stof · 2014-05-17T18:43:06Z

I suspect your meant to open the PR against 2.3

Tobion · 2014-05-17T18:56:19Z

No since it's a bc break.

fabpot · 2014-05-17T19:35:03Z

@Tobion But you have a bunch of unrelated commits in this PR.

Tobion · 2014-05-17T19:51:26Z

Because 2.3 is not merged in master yet.

fabpot · 2014-05-17T20:49:25Z

@Tobion 2.3 has now been merged into master.

ggam · 2014-05-18T08:56:16Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

     */
    private $pdo;

    /**
+     * @var string|null|false DNS string or null for session.save_path or false when lazy connection disabled


Maybe you meant DSN instead of DNS?

Tobion · 2014-05-19T00:03:13Z

This would be ready. Where should I document the change (depends for which version it gets merged)?

ggam · 2014-05-19T11:47:50Z

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php

@@ -51,17 +51,68 @@ public function testInexistentTable()
        $storage->close();
    }

-    public function testReadWriteRead()
+    public function testWithLazyDnsConnection()


stof · 2014-05-19T11:56:46Z

@Tobion I think similar changes should be done in the DBAL session handler.
The lazy connection is useless there as DBAL itself is lazy, and the table creation uses the DBAL schema system already, but the switch to BLOB would make sense for consistency

Tobion · 2014-05-21T14:49:08Z

@stof Yes I can do that in a later PR when we know whether #10908 stays in 2.3 or not.

Tobion · 2014-05-22T12:23:17Z

@stof you know what fabbot.io is complaining about? http://fabbot.io/report/symfony/symfony/10931/953226bdb666880c61157a42d7839eb42c9c8c9c

vntw · 2014-05-22T12:27:02Z

There are some extra spaces after the ';'

Tobion · 2014-05-25T02:03:24Z

@venyii Thanks for spotting

Tobion · 2014-05-25T12:32:54Z

Reopened against 2.5: #10991

Tobion · 2014-05-27T08:01:49Z

Ok, master 2.6 again.

Tobion · 2014-05-27T08:26:46Z

@fabpot ready

Tobion · 2014-05-27T08:37:27Z

I'll write a blog post about this soon.

staabm · 2014-05-27T08:48:47Z

@Tobion please cross link your post here

Tobion · 2014-05-29T16:13:50Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

+     * @throws \PDOException    When the table already exists
+     * @throws \DomainException When an unsupported PDO driver is used
+     */
+    public function createTable()


I need to add a check that we are not in a transaction, because it would implicit commit it and thus ruin the locking when called after read(). http://dev.mysql.com/doc/refman/5.0/en/implicit-commit.html

Hm doesn't really matter since as read() would raise an error anyway when the table doesn't exist. But to be correct rollback should be called.

FYI, PostgreSQL actually supports performing DDL changes in a transaction, and rolling back these changes a whole. Its one of the many reasons why I love PostgreSQL ;)

So if you create a table inside a transaction the table will not be visible outside of the transaction, but the pg_class catalogs tables (and related) are however locked until the transaction is released.

Not a problem in this case, just some useful information :)

Thanks for the information

Tobion · 2014-09-29T19:33:40Z

@fabpot this is ready for merge.

I tested the binary data and the different locking strategies with both mysql and sqlite. It works like a charm. But I don't have postgres, oracle, mssql available. So it would be good if the community can test the class with these DBMS. Or I might find time to set it up later.
I would propose to merge this and then we can still easily fix it if there are problems with binary data for postgres/oracle/mysql.

Tobion · 2014-09-29T19:41:49Z

@symfony/deciders ping

staabm · 2014-09-29T19:43:19Z

@Tobion should one of the locking strategies be recommended for the "regular-user"?

Tobion · 2014-09-29T19:48:38Z

The default strategy (transactional) is recommended because it's most reliable across dbms.

romainneutron · 2014-09-29T19:51:21Z

The documentation could enjoy an update in this section: http://symfony.com/doc/current/cookbook/configuration/pdo_session_storage.html#sharing-your-database-connection-information

As it's not recommended anymore to share the doctrine connection, we should deprecate this part

romainneutron · 2014-09-29T19:55:28Z

What about an auto-save parameter in the framework bundle (in the session section)?
I mean, we could auto save the session on an event (for example Kernel::REQUEST) to release the lock and prevent locking simultaneous ajax requests.
This auto-save could be disabled on some route using a annotation.

Tobion · 2014-09-29T19:56:13Z

@romainneutron this section is only about sharing the connection settings. Not the connection itself. So this part is still ok.

romainneutron · 2014-09-29T19:57:14Z

@romainneutron this section is only about sharing the connection settings. Not the connection itself. So this part is still ok.

woops, you're right

Tobion · 2014-09-29T20:15:46Z

I think there are different cases that need to be examined, e.g. an AJAX request to:

a non-secured path without firewall and a controller without session access: nothing to do as no blocking is involved
a secured path and a controller with session access: we cannot improve anything. users should call session->save() before they do some other heavy stuff (in case they do).
a secured path and a controller without session access: AFAIK the security bundle accesses session data. so in theory the session could be closed right after that, so the lock does not cover the time in the controller where the session is not used.

stof · 2014-09-30T17:20:23Z

a secured path and a controller with session access: we cannot improve anything. users should call session->save() before they do some other heavy stuff (in case they do).

Maybe we could add a listener on kernel.terminate with a high priority, to save the session before the actions done after the response sending are processed. This would solve the issue for the common case triggered by the memory spool delaying the session saving for instance

staabm · 2014-09-30T17:23:21Z

What about some sort of debugging mechanism (e.g. A class using ArrayAccess) which will throw exceptions in case a code snippet tries to add/manipulate session data after the session has already been written+closed?

Tobion · 2014-09-30T17:39:19Z

@stof yes that is what I proposed in #6417 to fix that issue.
@staabm Zend Session has this feature as far as I remember. But its not related to this PR.
Please just focus on this PR and discuss other things in other tickets.

Tobion · 2014-10-02T14:38:09Z

Any votes?

fabpot · 2014-10-02T15:43:23Z

src/Symfony/Component/HttpFoundation/CHANGELOG.md

+ * PdoSessionHandler changes
+   - implemented different session locking strategies to prevent loss of data by concurrent access to the same session
+   - save session data in a binary column without base64_encode
+   - added lifetime column to the session table which allows to have different lifetimes for each session


You should prefix this with [BC BREAK]

fabpot · 2014-10-02T15:43:43Z

👍

Tobion · 2014-10-03T00:25:05Z

I just realize this also fixes #9029

fabpot · 2014-10-03T05:27:06Z

Thank you so much for working on this @Tobion. This is much appreciated.

This PR was merged into the 2.6-dev branch. Discussion ---------- [HttpFoundation] enhance PdoSessionHandler | Q | A | ------------- | --- | Bug fix? | yes | New feature? | yes | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | #5483, #2067, #2382, #9029 | License | MIT 0. [x] Continuation of locking implementation (#10908): Implement different locking strategies - `PdoSessionHandler::LOCK_TRANSACTIONAL` (default): Issues a real row lock but requires a transaction - `PdoSessionHandler::LOCK_ADVISORY`: app-level lock, safe as long as only the PdoSessionHandler accesses sessions, advantage is it does not require a transaction (not implemented for oracle or sqlsrv yet) - `PdoSessionHandler::LOCK_NONE`: basically what is was before, prone to race conditions, means the last session write wins 1. [x] Save session data as binary: Encoding session data was definitely the wrong solution. Session data is binary text (esp. when using other session.serialize_handler) that must stay as-is and thus must also be safed in a binary column. Base64 encoding session data just decreses performance and increases storage costs and is semantically wrong because it does not have a character encoding. That saving null bytes in Posgres won't work on a character column is also documented > First, binary strings specifically allow storing octets of value zero and other "non-printable" octets (usually, octets outside the range 32 to 126). Character strings disallow zero octets, and also disallow any other octet values and sequences of octet values that are invalid according to the database's selected character set encoding. http://www.postgresql.org/docs/9.1/static/datatype-binary.html#DATATYPE-BINARY-TABLE 2. [x] Implement lazy connections that are only opened when session is used by either passing a dsn string explicitly or falling back to session.save_path ini setting. Fixes #9029 3. [x] add a create table method that creates the correct table depending on database vendor. This makes the class self-documenting and standalone useable. 5. [x] add lifetime column to session table which allows to have different lifetimes for each session 6. [x] add isSessionExpired() method to be able to distinguish between a new session and one that expired due to inactivity, e.g. to display flash message to user 7. [x] added upgrade and changelog notes Commits ------- 1bc6680 [HttpFoundation] implement different locking strategies for sessions 6f5748e adjust sqlite table definition 5978fcf added upgrade and changelog notes for PdoSessionHandler 182a5d3 [HttpFoundation] add create table method to pdo session handler e79229d [HttpFoundation] allow different lifetime per session af1bb1f add test for null byte in session data 251238d [HttpFoundation] implement lazy connect for pdo session handler 7dad54c [HttpFoundation] remove base64 encoding of session data

rybakit · 2014-10-03T09:47:33Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

+                return '';
+            }
+
+            return $sessionRows[0][0];


I guess it will not work at least for PostgreSQL, as the bytea fields are returned as streams for this driver: http://php.net/manual/en/ref.pdo-pgsql.connection.php

Good catch, I know Travis-ci supports both PostgreSQL and MySQL so maybe we can add an general integration test to make sure all the drivers are working properly?

Ping @Tobion

@rybakit As I said, I only tested it with mysql and sqlite. I would appreciate if you can test it with postgres and provide a fix.

A unit test with postgres would help for the simple case. But for the locking behavior we would need functional tests with simultaneous connections which is not that easy to do.

@Tobion I've submitted a fix #12146 with simple unit tests. But I agree that to test it properly we need functional tests for each driver.

Btw, did you consider to split PdoSessionHandler into SqlitePdoSessionHandler, MysqlPdoSessionHandler etc. (or create adapters) to ease tuning and remove all those if ('sqlite|mysql|...' === $this->driver) checks?

Yes I considered that. But then we would probably need to an abstract implementation with all the shared code and also change the pdoeessionhandler to be some sort of proxy, because you can still pass a pdo instance and then need to determine which concrete handler to use. Not sure if that makes things easier.

…with streams (rybakit) This PR was squashed before being merged into the 2.6-dev branch (closes #12146). Discussion ---------- [HttpFoundation] Fix PdoSessionHandler to work properly with streams | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT Ref: #10931 (comment) Commits ------- 9531a2b [HttpFoundation] Fix PdoSessionHandler to work properly with streams

bestform · 2015-07-22T11:10:21Z

@Tobion hey, i just wanted to come back at you about this and thank you for implementing our suggestions. It helped us immensely and made it possible to stay up to date with the latest symfony versions.

Especially exposing the lock strategies made it possible that we can utilize our session implementation.

So, just a quick thank you for your work and for listening and seeking a constructive discussion (see #10908)

(I know, it has been a while, but as I just updated our system to be based on symfony 2.7 I realized just how much this change helped)

ggam reviewed May 18, 2014
View reviewed changes

Tobion changed the title ~~[WIP][HttpFoundation] remove base64 encoding of session data~~ [WIP][HttpFoundation] save session data as binary and add lazy-connect / create table / lifetime per session May 18, 2014

Tobion changed the title ~~[WIP][HttpFoundation] save session data as binary and add lazy-connect / create table / lifetime per session~~ [WIP][HttpFoundation] save session data as binary & add lazy-connect & create table & lifetime per session May 18, 2014

Tobion changed the title ~~[WIP][HttpFoundation] save session data as binary & add lazy-connect & create table & lifetime per session~~ [HttpFoundation] save session data as binary & add lazy-connect & create table & lifetime per session May 18, 2014

Tobion referenced this pull request May 19, 2014

[HttpFoundation] implement session locking for PDO

50ec828

anlutro mentioned this pull request May 19, 2014

Database session driver incompatibilities laravel/framework#4441

Closed

ggam reviewed May 19, 2014
View reviewed changes

Tobion mentioned this pull request May 23, 2014

[HttpFoundation] implement session locking for PDO #10908

Merged

Tobion closed this May 25, 2014

Tobion reopened this May 27, 2014

Tobion reviewed May 29, 2014
View reviewed changes

Tobion force-pushed the binary-session-data branch from 9ca183f to c900de5 Compare September 29, 2014 19:23

Tobion changed the title ~~[WIP][HttpFoundation] enhance PdoSessionHandler~~ [HttpFoundation] enhance PdoSessionHandler Sep 29, 2014

fabpot reviewed Oct 2, 2014
View reviewed changes

[HttpFoundation] implement different locking strategies for sessions

1bc6680

Tobion force-pushed the binary-session-data branch from c900de5 to 1bc6680 Compare October 2, 2014 18:16

Tobion mentioned this pull request Oct 3, 2014

[HttpFoundation] PdoSessionHandler should connect to database only if session required #9029

Closed

fabpot merged commit 1bc6680 into symfony:master Oct 3, 2014

Tobion deleted the binary-session-data branch October 3, 2014 09:15

rybakit reviewed Oct 3, 2014
View reviewed changes

rybakit mentioned this pull request Oct 6, 2014

[HttpFoundation] Fix PdoSessionHandler to work properly with streams #12146

Closed

Tobion mentioned this pull request Nov 2, 2014

[WIP] Added DBAL session storaged based on PDO Session storage page symfony/symfony-docs#3914

Closed

[HttpFoundation] enhance PdoSessionHandler #10931

[HttpFoundation] enhance PdoSessionHandler #10931

Conversation

Tobion commented May 17, 2014

stof commented May 17, 2014

Tobion commented May 17, 2014

fabpot commented May 17, 2014

Tobion commented May 17, 2014

fabpot commented May 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented May 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented May 19, 2014

Tobion commented May 21, 2014

Tobion commented May 22, 2014

vntw commented May 22, 2014

Tobion commented May 25, 2014

Tobion commented May 25, 2014

Tobion commented May 27, 2014

Tobion commented May 27, 2014

Tobion commented May 27, 2014

staabm commented May 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Sep 29, 2014

Tobion commented Sep 29, 2014

staabm commented Sep 29, 2014

Tobion commented Sep 29, 2014

romainneutron commented Sep 29, 2014

romainneutron commented Sep 29, 2014

Tobion commented Sep 29, 2014

romainneutron commented Sep 29, 2014

Tobion commented Sep 29, 2014

stof commented Sep 30, 2014

staabm commented Sep 30, 2014

Tobion commented Sep 30, 2014

Tobion commented Oct 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Oct 2, 2014

Tobion commented Oct 3, 2014

fabpot commented Oct 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bestform commented Jul 22, 2015