[HttpFoundation] enhance PdoSessionHandler #10931

Merged
merged 8 commits into from Oct 3, 2014

Conversation

Projects
None yet
Member

Tobion commented May 17, 2014

Q A
Bug fix? yes
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #5483, #2067, #2382, #9029
License MIT
  1. 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
  2. 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

  3. 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

  4. add a create table method that creates the correct table depending on database vendor. This makes the class self-documenting and standalone useable.

  5. add lifetime column to session table which allows to have different lifetimes for each session

  6. 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. added upgrade and changelog notes

Member

stof commented May 17, 2014

I suspect your meant to open the PR against 2.3

Member

Tobion commented May 17, 2014

No since it's a bc break.

Owner

fabpot commented May 17, 2014

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

Member

Tobion commented May 17, 2014

Because 2.3 is not merged in master yet.

Owner

fabpot commented May 17, 2014

@Tobion 2.3 has now been merged into master.

*/
private $pdo;
+ /**
+ * @var string|null|false DNS string or null for session.save_path or false when lazy connection disabled
@ggam

ggam May 18, 2014

Maybe you meant DSN instead of DNS?

@Tobion

Tobion May 18, 2014

Member

haha true

*/
private $pdo;
+ /**
+ * @var string|null|false DNS string or null for session.save_path or false when lazy connection disabled
+ */
+ private $dns = false;
@staabm

staabm May 18, 2014

Contributor

Same typo in the var

@Tobion

Tobion May 18, 2014

Member

same typo everywhere. I'm consistent :)

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

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

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

Member

Tobion commented May 19, 2014

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

Tobion referenced this pull request May 19, 2014

@anlutro anlutro referenced this pull request in laravel/framework May 19, 2014

Closed

Database session driver incompatibilities #4441

@@ -51,17 +51,68 @@ public function testInexistentTable()
$storage->close();
}
- public function testReadWriteRead()
+ public function testWithLazyDnsConnection()
- $storage = new PdoSessionHandler($this->pdo);
+ $dbFile = tempnam(sys_get_temp_dir(), 'sf2_sqlite_sessions');
+ if (file_exists($dbFile)) {
+ @unlink($dbFile);
@staabm

staabm May 19, 2014

Contributor

should the test be skipped when unlink is not successfull?

@Tobion

Tobion May 21, 2014

Member

I'd rather get an error the following call.

+ $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol VARBINARY(MAX) NOT NULL, $this->lifetimeCol INTEGER NOT NULL, $this->timeCol INTEGER NOT NULL)";
+ break;
+ default:
+ return;
@stof

stof May 19, 2014

Member

shouldn't it throw an exception saying that the driver is not supported instead ?

Member

stof commented May 19, 2014

@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

Member

Tobion commented May 21, 2014

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

venyii commented May 22, 2014

There are some extra spaces after the ';'

Member

Tobion commented May 25, 2014

@venyii Thanks for spotting

Member

Tobion commented May 25, 2014

Reopened against 2.5: #10991

@Tobion Tobion closed this May 25, 2014

Member

Tobion commented May 27, 2014

Ok, master 2.6 again.

@Tobion Tobion reopened this May 27, 2014

Member

Tobion commented May 27, 2014

@fabpot ready

Member

Tobion commented May 27, 2014

I'll write a blog post about this soon.

Contributor

staabm commented May 27, 2014

@Tobion please cross link your post here

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

Tobion May 29, 2014

Member

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

@Tobion

Tobion May 31, 2014

Member

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.

@sstok

sstok Jun 1, 2014

Contributor

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 :)

@Tobion

Tobion Jun 1, 2014

Member

Thanks for the information

+
+ switch ($this->driver) {
+ case 'mysql':
+ $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->lifetimeCol MEDIUMINT NOT NULL, $this->timeCol INTEGER NOT NULL) ENGINE = InnoDB";
@Tobion

Tobion Jun 2, 2014

Member

Collation of the id column should be utf8_bin because we don't want it case-insensitive nor handling language processing like é == e.

@Tobion

Tobion Jun 2, 2014

Member

We don't even need multibyte chars from urf8, so ascii would be enough. Actually we can just as well use varbinary which would also prevent unnecessary character set conversions between server and client and trailing space removal. http://dev.mysql.com/doc/refman/5.5/en/charset-binary-collations.html

@Tobion

Tobion Jun 5, 2014

Member

also i can use unsigned for the time column

@Tobion Tobion changed the title from [HttpFoundation] save session data as binary & add lazy-connect & create table & lifetime per session to [WIP][HttpFoundation] save session data as binary & add lazy-connect & create table & lifetime per session Jun 4, 2014

+ $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->lifetimeCol MEDIUMINT NOT NULL, $this->timeCol INTEGER NOT NULL) ENGINE = InnoDB";
+ break;
+ case 'sqlite':
+ $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->lifetimeCol MEDIUMINT NOT NULL, $this->timeCol INTEGER NOT NULL)";
@Tobion

Tobion Jun 4, 2014

Member

there is no varchar(...) or mediumint in sqlite because its dynamically typed. I can just use text and integer

@Tobion Tobion changed the title from [WIP][HttpFoundation] save session data as binary & add lazy-connect & create table & lifetime per session to [WIP][HttpFoundation] enhance PdoSessionHandler Jun 10, 2014

Owner

fabpot commented Jun 15, 2014

@Tobion This king of changes should really be merged early in the 2.6 process so that we have plenty of time to test it in real-world situations. @Tobion Do you think it can be ready soon?

Member

Tobion commented Jun 15, 2014

@fabpot yes almost done

Owner

fabpot commented Sep 23, 2014

@Tobion Any news on this one? Any chance we can merge it this week? That would let us 2 months to be sure it works well for everyone before 2.6 final.

Member

Tobion commented Sep 23, 2014

I don't think I can make it this week because I'm on holiday. But I'll finish it next Monday.

Owner

fabpot commented Sep 23, 2014

Monday would be perfect, thanks.

+
+ switch ($this->driver) {
+ case 'mysql':
+ $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->lifetimeCol MEDIUMINT NOT NULL, $this->timeCol INTEGER UNSIGNED NOT NULL) ENGINE = InnoDB";
@Tobion

Tobion Sep 23, 2014

Member

change id column to varbinary which also works

@Tobion Tobion changed the title from [WIP][HttpFoundation] enhance PdoSessionHandler to [HttpFoundation] enhance PdoSessionHandler Sep 29, 2014

Member

Tobion commented Sep 29, 2014

@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.

Member

Tobion commented Sep 29, 2014

Contributor

staabm commented Sep 29, 2014

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

Member

Tobion commented Sep 29, 2014

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

Member

romainneutron commented Sep 29, 2014

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

Member

romainneutron commented Sep 29, 2014

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.

Member

Tobion commented Sep 29, 2014

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

Member

romainneutron commented Sep 29, 2014

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

woops, you're right

Member

Tobion commented Sep 29, 2014

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.
Member

stof commented Sep 30, 2014

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

Contributor

staabm commented Sep 30, 2014

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?

Member

Tobion commented Sep 30, 2014

@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.

Member

Tobion commented Oct 2, 2014

Any votes?

nevermind, you fixed that in another commit.

+ * 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
@fabpot

fabpot Oct 2, 2014

Owner

You should prefix this with [BC BREAK]

Owner

fabpot commented Oct 2, 2014

👍

Member

Tobion commented Oct 3, 2014

I just realize this also fixes #9029

Owner

fabpot commented Oct 3, 2014

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

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

0 of 2 checks passed

continuous-integration/travis-ci The Travis CI build failed
Details
default Success: fabbot — Failure: Travis
Details

fabpot added a commit that referenced this pull request Oct 3, 2014

feature #10931 [HttpFoundation] enhance PdoSessionHandler (Tobion)
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

@Tobion Tobion deleted the Tobion:binary-session-data branch Oct 3, 2014

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

rybakit Oct 3, 2014

Contributor

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

@sstok

sstok Oct 3, 2014

Contributor

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?

@Tobion

Tobion Oct 5, 2014

Member

@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.

@rybakit

rybakit Oct 6, 2014

Contributor

@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?

@Tobion

Tobion Oct 6, 2014

Member

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.

fabpot added a commit that referenced this pull request Oct 7, 2014

minor #12146 [HttpFoundation] Fix PdoSessionHandler to work properly …
…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

Wow this is awesome. Is there a plan to port this to DbalSessionHandler? We use that since we don't need to configure database connections again and it also provides schema for doctrine.

Member

Tobion replied Nov 13, 2014

Yes it's the goal. Just somebody needs to find time for it. FYI, with transactional locking you should not use the same DB connection your already use for your application logic.

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment