[HttpFoundation] MongoDbSessionHandler::read() now checks for valid session age #12516

bzikarsky · Nov 19, 2014

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets	-
License	MIT
Doc PR	-

As discussed with @jmikola:

It's a bugfix - expired sessions should never be read
Therefore should be merged into 2.3 LTS
TODO: Modify the behavior to match the PDO adapter
TODO: Add a precise description of the behavior to the manual

-- old description below --

This patch can be seen as either bugfix or feature, though it's probably more on the feature-side.

Currently a session will be returned by read() even if it's already older than gc_maxlifetime. If gc_maxlifetime is used for timing out sessions and a low gc-ratio is in place there is a high chance the session stays alive for a longer time than wanted. The manual describes the current limitations of gc_maxlifetime correctly at http://symfony.com/doc/current/components/http_foundation/session_configuration.html#session-lifetime.

But since the modification timestamp is stored in MongoDB anway, we can actually use it to only return "valid" session-data as does the PdoSessionHandler

pborreli · Nov 19, 2014

could this addition affect performance ? did you compare with just testing the date after fetching the record ? ping @jmikola

bzikarsky · Nov 20, 2014

Initially I only did some testing regarding the scanning-behavior: Mongo hits the indexed _id field, and scans all matching documents (1 😄) for the $gt condition.

Since you asked I threw together a quick benchmark: https://gist.github.com/bzikarsky/26706edc07d5e4cceb05

The results are inconclusive. (Ran it on a rather slow single core VPS)

n_sess	n_read	t_mongo	t_php
1000	1000	0.11557793617249s	0.11554098129272s
10000	1000	0.11895108222961s	0.12062788009644s
100000	1000	0.12337493896484s	0.12323498725891s
100000	10000	1.2311668395996s	1.1953091621399s
1000000	100000	13.634443998337s	12.997648954391s

I assume the db tuning makes all the difference between PHP/Mongo. I think it's simpler to push the condition into the query, but if the majority prefers a PHP condition, I'm happy to update the PR accordingly 😄

pborreli · Nov 20, 2014

nice, thank you very much for this benchmark ! did you ran it against a large session dataset ?

bzikarsky · Nov 20, 2014

Well, 1.000.000 artificial sessions with 100.000 reads seemed big enough for me, or do you mean something else?

pborreli · Nov 20, 2014

no it's great already, it gives us a very good information about how it affects performance.
Let's see others feedback on this PR, thanks @bzikarsky

stof · Nov 28, 2014

@Drak does this makes sense ?

jmikola · Nov 29, 2014

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

@@ -159,8 +159,11 @@ public function write($sessionId, $data)
     */
    public function read($sessionId)
    {
+        $timeoutThreshold = new \MongoDate(time() - (int) ini_get('session.gc_maxlifetime'));


I believe there is a bug with the current code where read() may return an expired session (if gc() is not invoked).

If we compare this to PdoSessionHandler, its doRead() method checks the timestamp and lifetime values after selecting the record from the database. If the INI value is changed, I think the correct behavior would be to respect the original lifetime from the session. I suppose it would be updated when we next wrote the session back to the database.

Currently, the MongoDB handler only stores lifetime indirectly in a calculated expiry field (useful for the TTL indexing). To properly fix this, I think we may need to store the lifetime in a new field at all times.

jmikola · Nov 29, 2014

@Drak: Speaking to @bzikarsky in IRC, we noted that the documentation doesn't really discuss how sessions are timed out. Perhaps this is only an issue for the database handlers, where this kind of expiration logic is implemented in PHP (in contrast to Memcache using its own TTL, or the filesystem handler). Any thoughts on that?

bzikarsky · Nov 29, 2014

That would break BC for configurations relying on TTL indices though. In the gc-method we have a check that tests for !== false and skips garbage-collection for truthy values.

a) We remove the gc-optimization. This only introduces an additional query for people with TTL indices every $gcProbability requests and can be avoided by setting gc-probability to 0. (My preference)

b) We introduce another option for TTL-indices (actual BC break)

c) We use a separate option for the new expiry-field, but this gets messy really fast (I can't even think of a good name for this option...)

PR is now at an implementation of a). I moved all inline documentation regarding the TTL collections to the constructor-docblock. We probably should update the documentation with some of the insights.

timglabisch · Nov 29, 2014

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

@@ -127,24 +132,14 @@ public function gc($maxlifetime)
     */
    public function write($sessionId, $data)
    {
+        $expiry = new \MongoDate(time() + (int) ini_get('session.gc_maxlifetime'));


we should try to inject time() also. correct me if i am wrong, but this assumes that your webserver and your database server are sharing the same timezome?

It's been this way. I think this should be a separate issue?

Edit: MongoDate is not timezone aware anyway. So this is a non-issue. It's always based on PHP's current default-timezone. It's the driver's responsibility to "translate"

Correction, it's based on the timezone of PHP's execution context, see: https://gist.github.com/bzikarsky/c309a55fefbaf9bdaa5d

Anyhow: ext-mongo uses the system's timezone to create correct Isodate-instances in the database. The same is valid for reading.

Ignoring PHP's timezone is probably a bug. But that is located in ext-mongo.

i looked at the other session handlers, this issue is related to most of them. to solve this issue symfony should provide a service that is able to return date / time informations.

Ignoring PHP's timezone is probably a bug. But that is located in ext-mongo.

I would disagree. The BSON date type is specified as:

UTC datetime - The int64 is UTC milliseconds since the Unix epoch.

In your gist, strtotime() happens to respect PHP's timezone, as does date('c') when you print out the dates later; however, when printing the date on the way out of the database, you're ignoring the original timezone from the tz field. PHP will simply use the UNIX timestamp from the MongoDate and render it in the default timezone. This is why the time offsets of the three records are still staggered (0, +2, and +1 respectively). Does that make sense?

Just to avoid any more confusion: I'm fine with the output of my exmaple-script. This is what I expected. My comment about there being a bug in ext-mongo was related to me falling back to the usage of TZ=/usr/share/zoneinfo.... But I must have tricked myself somewhere.

I was under the impression that ext-mongo converts the UNIX timestamps to Isodates (in UTC) based on the TZ process context, and ignores PHP's internal date.timezone. That's the reason why I went for a bash-script instead of a PHP script in the beginning.

But I ran a 2nd test today morning, and this time it worked flawlessly. Strange. Modified Gist at https://gist.github.com/bzikarsky/4044d1fa37e32e5c6996

Anyhow I think everything is fine - no bugs anywhere to be seen. 😉

bzikarsky · Dec 1, 2014

How do we continue from here on? With the current solution I propose several changes to the manual:

Clarify behavior for "timed out" (older than gc_maxlifetime) session on read - maybe push a strict "Session older than gc_maxlifetime are never read"-phrase; we need to check the other session ahndlers before though
Clarify session lifetime behavior regarding changes to gc_maxlifetime when sessions already exist:
- Sessions are updated every request with the new gc.max_lifetime
- Idle sessions (as in: inactive) are not updated to the new lifetime, and times out according to the old lifetime
Add a section to the MongoSessionHandler-documentation about proper index-configuration with and without TTL indices.

(And I don't know how to handle the test-failures which are unrelated to my changes - do I rebase on 2.7 when those are fixed?)

jmikola · Dec 1, 2014

Add a section to the MongoSessionHandler-documentation about proper index-configuration with and without TTL indices.

AFAIK, the documentation doesn't have specific pages on these session hander implementations. They are only mentioned in passing on the Session Configuration page within the HttpFoundation chapter of the book.

It looks like we could easily create a new documentation page. Instead of linking to the API documentation for each provider, we could create new pages as needed. MongoDB's might be the first.

fabpot · Jan 3, 2015

What's the status of this PR?

bzikarsky · Jan 4, 2015

It's idle. I was waiting on feedback for the proposed manual changes - especially regarding the "Sessions older than gc_maxlifetime are never read"-stance - and then forgot about it.

If the general direction of this PR and the proposed documentation-changes is approved, I 'll prepare a PR on documentation and check behaviour of the other session-handlers.

jmikola · Jan 7, 2015

@fabpot, @bzikarsky: I concur that this is just waiting on a documentation PR. I concur with the documentation todo items in this comment. The generic topics about "timed out" sessions being read and session lifetimes being updated on each request can appear within Session Configuration.

Personally, I'm not sure why Session Cookie Lifetime and Session Lifetime are split (Configuring Garbage Collection appears between them). Both sections talk about session cookies. I do think that Session Lifetime is the better place to discuss the topics relevant to this PR.

As for documenting suggested MongoDB indexes, that does belong on a new page, which should be linked to from Custom Save Handlers.

fabpot · Feb 5, 2015

I'm going to merge it as is then.

@bzikarsky Can you submit a PR for the docs?

fabpot · Feb 5, 2015

I've just noticed that this is a bug fix that needs to be merged in 2.3. Can you submit a PR for that branch?

Initial PR was on 2.7: Conflicts: src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php

bzikarsky · Mar 12, 2015

I'll close this in favour for the new PR (based on 2.3) - #13911

…for valid session age (bzikarsky) This PR was squashed before being merged into the 2.3 branch (closes #13911). Discussion ---------- [HttpFoundation] MongoDbSessionHandler::read() now checks for valid session age This PR is a follow-up to #12516 and replaces the old one. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | - | License | MIT | Doc PR | TODO As discussed there: Sessions which are older than GC age should never be read. This PR adds the expiry-datetime on session-write and changes session-read and session-gc accordingly. We still need to update the documentation with some clarifications, as described here: - #12516 (comment) - #12516 (comment) My experience with the Symfony Docs from a developer perspective is very limited, so help would be very appreciated. Commits ------- 8289ec3 [HttpFoundation] MongoDbSessionHandler::read() now checks for valid session age

MongoDbSessionHandler::read() now checks for valid session age

3f6372d

bzikarsky force-pushed the mongodb-session-safe-read branch from 51a3db1 to 3f6372d Compare November 19, 2014 14:21

bzikarsky changed the title ~~MongoDbSessionHandler::read() now checks for valid session age~~ [HttpFoundation] MongoDbSessionHandler::read() now checks for valid session age Nov 20, 2014

jmikola reviewed Nov 29, 2014
View reviewed changes

Session expiry date is now defined on session-write

a3fbf34

bzikarsky force-pushed the mongodb-session-safe-read branch from 52cb779 to a3fbf34 Compare November 29, 2014 17:19

timglabisch reviewed Nov 29, 2014
View reviewed changes

fabpot added the HttpFoundation label Dec 7, 2014

fabpot force-pushed the 2.7 branch from 77b6cca to dcfc338 Compare January 8, 2015 13:06

bzikarsky mentioned this pull request Mar 12, 2015

[HttpFoundation] MongoDbSessionHandler::read() now checks for valid session age #13911

Closed

bzikarsky closed this Mar 12, 2015

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

Uh oh!

[HttpFoundation] MongoDbSessionHandler::read() now checks for valid session age #12516

[HttpFoundation] MongoDbSessionHandler::read() now checks for valid session age #12516

Uh oh!

Conversation

bzikarsky commented Nov 19, 2014

Uh oh!

pborreli commented Nov 19, 2014

Uh oh!

bzikarsky commented Nov 20, 2014

Uh oh!

pborreli commented Nov 20, 2014

Uh oh!

bzikarsky commented Nov 20, 2014

Uh oh!

pborreli commented Nov 20, 2014

Uh oh!

stof commented Nov 28, 2014

Uh oh!

jmikola Nov 29, 2014

Choose a reason for hiding this comment

Uh oh!

jmikola commented Nov 29, 2014

Uh oh!

bzikarsky commented Nov 29, 2014

Uh oh!

timglabisch Nov 29, 2014

Choose a reason for hiding this comment

Uh oh!

bzikarsky Nov 29, 2014

Choose a reason for hiding this comment

Uh oh!

bzikarsky Nov 29, 2014

Choose a reason for hiding this comment

Uh oh!

timglabisch Nov 29, 2014

Choose a reason for hiding this comment

Uh oh!

jmikola Nov 30, 2014

Choose a reason for hiding this comment

Uh oh!

bzikarsky Nov 30, 2014

Choose a reason for hiding this comment

Uh oh!

bzikarsky commented Dec 1, 2014

Uh oh!

jmikola commented Dec 1, 2014

Uh oh!

fabpot commented Jan 3, 2015

Uh oh!

bzikarsky commented Jan 4, 2015

Uh oh!

jmikola commented Jan 7, 2015

Uh oh!

fabpot commented Feb 5, 2015

Uh oh!

fabpot commented Feb 5, 2015

Uh oh!

bzikarsky commented Mar 12, 2015

Uh oh!

Uh oh!