Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[HttpFoundation] enhance PdoSessionHandler#10931

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 8 commits intosymfony:masterfromTobion:binary-session-data
Oct 3, 2014

Conversation

@Tobion
Copy link
Contributor

QA
Bug fix?yes
New feature?yes
BC breaks?yes
Deprecations?no
Tests pass?yes
Fixed tickets#5483,#2067,#2382,#9029
LicenseMIT

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

  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[HttpFoundation] PdoSessionHandler should connect to database only if session required #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

@stof
Copy link
Member

I suspect your meant to open the PR against 2.3

@Tobion
Copy link
ContributorAuthor

No since it's a bc break.

@fabpot
Copy link
Member

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

@Tobion
Copy link
ContributorAuthor

Because 2.3 is not merged in master yet.

@fabpot
Copy link
Member

@Tobion 2.3 has now been merged into master.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Maybe you meantDSN instead ofDNS?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

haha true

@TobionTobion 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 sessionMay 18, 2014
@TobionTobion 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 sessionMay 18, 2014
@TobionTobion 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 sessionMay 18, 2014
@Tobion
Copy link
ContributorAuthor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

dsn*

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

thx fixed

@stof
Copy link
Member

@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
Copy link
ContributorAuthor

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

@Tobion
Copy link
ContributorAuthor

@vntw
Copy link

There are some extra spaces after the ';'

@Tobion
Copy link
ContributorAuthor

@venyii Thanks for spotting

@Tobion
Copy link
ContributorAuthor

Reopened against 2.5:#10991

@TobionTobion closed thisMay 25, 2014
@Tobion
Copy link
ContributorAuthor

Ok, master 2.6 again.

@TobionTobion reopened thisMay 27, 2014
@Tobion
Copy link
ContributorAuthor

@fabpot ready

@Tobion
Copy link
ContributorAuthor

I'll write a blog post about this soon.

@staabm
Copy link
Contributor

@Tobion please cross link your post here

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for the information

@TobionTobion changed the title[WIP][HttpFoundation] enhance PdoSessionHandler[HttpFoundation] enhance PdoSessionHandlerSep 29, 2014
@Tobion
Copy link
ContributorAuthor

@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
Copy link
ContributorAuthor

@symfony/deciders ping

@staabm
Copy link
Contributor

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

@Tobion
Copy link
ContributorAuthor

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

@romainneutron
Copy link
Contributor

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
Copy link
Contributor

What about anauto-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
Copy link
ContributorAuthor

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

@romainneutron
Copy link
Contributor

@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
Copy link
ContributorAuthor

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
Copy link
Member

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 onkernel.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
Copy link
Contributor

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
Copy link
ContributorAuthor

@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
Copy link
ContributorAuthor

Any votes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You should prefix this with[BC BREAK]

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

done

@fabpot
Copy link
Member

👍

@Tobion
Copy link
ContributorAuthor

I just realize this also fixes#9029

@fabpot
Copy link
Member

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

@fabpotfabpot merged commit1bc6680 intosymfony:masterOct 3, 2014
fabpot added a commit that referenced this pull requestOct 3, 2014
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       | MIT0. [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 wins1. [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-TABLE2. [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#90293. [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 session6. [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 user7. [x] added upgrade and changelog notesCommits-------1bc6680 [HttpFoundation] implement different locking strategies for sessions6f5748e adjust sqlite table definition5978fcf added upgrade and changelog notes for PdoSessionHandler182a5d3 [HttpFoundation] add create table method to pdo session handlere79229d [HttpFoundation] allow different lifetime per sessionaf1bb1f add test for null byte in session data251238d [HttpFoundation] implement lazy connect for pdo session handler7dad54c [HttpFoundation] remove base64 encoding of session data
@TobionTobion deleted the binary-session-data branchOctober 3, 2014 09:15
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@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 splitPdoSessionHandler intoSqlitePdoSessionHandler,MysqlPdoSessionHandler etc. (or create adapters) to ease tuning and remove all thoseif ('sqlite|mysql|...' === $this->driver) checks?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 requestOct 7, 2014
…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       | MITRef:#10931 (comment)Commits-------9531a2b [HttpFoundation] Fix PdoSessionHandler to work properly with streams
@bestform
Copy link

@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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

11 participants

@Tobion@stof@fabpot@vntw@staabm@romainneutron@bestform@patrickli@rybakit@sstok@ggam
[8]ページ先頭
©2009-2025 Movatter.jp