Skip to content

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

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

Closed
tgabi333 opened this issue Sep 13, 2013 · 5 comments · Fixed by #10931
Closed

Comments

@tgabi333
Copy link
Contributor

PdoSessionHandler gets its PDO connection via constructor parameter and there is a known PDO behavior that it connects to database at instantiation.

This cause a bad application behavior that every request opens a connection to database and authorize db user even if there is no need to create a session. (Think about a site which requires sessions rarely)

This is not a case when using Memcached or Mongodb because they have the ability to connect on demand.

I have a PR in progress, but first i'd like to get a confirmation to continue the work with unit tests and documentation too.

@stof
Copy link
Member

stof commented Sep 13, 2013

the PDOSessionHandler constructor cannot be changed for BC reasons. It would have to be a separate class.

As we also have a Doctrine DBAL session handler and the doctrine connections connects on demand, I'm not sure we need to add a new PDO handler. @fabpot what do you think ?

@tgabi333
Copy link
Contributor Author

We could do it without breaking BC, widening the type of constructor parameter. It could be a \PDO object (still BC) and it could be an array of connection settings.

Anyway i think it is worth to mention n the documentation that there are a doctrine DBAL replacement.

@sstok
Copy link
Contributor

sstok commented Sep 14, 2013

You can try to extend the PDO class, and let __construct() only store the configuration.
But not make the actual connection.

Then when you call any of the actual methods you call connect().
Which internally detects if there is a connection, and if not will call parent::__construct() to init the actual connection.

This will make the PDO driver lazy without breaking anything.

Not sure if it would work trough.

@tgabi333
Copy link
Contributor Author

@sstok It would not work in this context because of PDO::setAttribute and PDO::getAttribute calls.

@Tobion
Copy link
Contributor

Tobion commented Oct 3, 2014

Will be fixed by #10931

fabpot added a commit that referenced this issue Oct 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       | 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants