[HttpFoundation][Sessions] Losing session in case of many concurrent requests. #4668

yanoosh · 2012-06-26T20:11:30Z

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT

When symfony handling many request of one session many instances try read and write session file. The first phase of writing the file is empty and then write session data. If script load an empty file then session will be incorrect. Easiest way to prevent this behaviour is use flock function which allows to control file usage.

travisbot · 2012-06-26T20:21:54Z

This pull request passes (merged ea1f7a5c into d0e1547).

vicb · 2012-06-27T06:12:07Z

Do you have a failing TC with concurrent access ?

As PHP is supposed to take care of serializing the requests (ie you can't have 2 concurrent requests for the same session) the issue might be related to PHP.

ghost · 2012-06-27T08:28:21Z

I suspected this would be the case, that's why the native files save handler is preferable.

vicb · 2012-06-27T08:33:02Z

@Drak what exactly did you suspect ?

If there could be concurrent requests for the same session then all non-native handler are flawed - but this this not be the case (as it is handled by PHP).

ghost · 2012-06-27T08:42:14Z

@vicb - Just that the locking is handled in the files handler see source but only for windows. I would also like to see a test case though to know if this is just theoretical or not.

yanoosh · 2012-06-27T19:23:51Z

Problem with session appear after last update (about 5 days ago). Earlier when Symfony used native PHP session handler everything was OK. I cant give you a concrete test case because this problem occurs randomly. Some time I can work on dev environment for 30 minutes and problem does not exist, but some time every refresh is crashed. Important thing is that the problem alway appear when browser load asset file (js, css, templates), never when load page (html). I use to load JavaScript and templates file a requirejs witch generate so many request.

I work all day on my patch and the problem did not appear. My dev PC work on linux Kubuntu 12.04. I dont test patch on Windows.

vicb · 2012-06-28T07:17:25Z

@yanoosh thanks for the update. I should have some time to investigate beginning of next week. What are the symptoms when it crashes ?

ooflorent · 2012-06-28T10:21:42Z

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

+        $success = (bool)fwrite($handle, $data);
+        flock($handle, LOCK_UN);
+        fclose($handle);
+        return $success;


Missing extra line before return

travisbot · 2012-06-28T18:37:23Z

This pull request fails (merged 12fddfe6 into 0d27570).

ghost · 2012-06-29T17:27:46Z

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

+            flock($handle, LOCK_UN);
+            fclose($handle);
+        } else {
+            $data = '';


I would put this $data = '';before the if (is_readable(...)) { in case the inner if returns false.

yanoosh · 2012-06-29T20:38:30Z

I created a repository which allows you to test my patch. https://github.com/yanoosh/symfony-session-test
By default, test work on Symfony without my changes.
Testing starts after opening the home page (sending a lot of ajax requests). Like I mentioned before the problem appear randomly so run page few time.

vicb · 2012-07-02T07:43:08Z

@yanoosh I used to test concurrency some weeks ago with this code - the handler comes for php.net.

I did not notice any problem: If you execute two concurrent requests, the second one will block until the first one is done (i.e. the session data have been written).

Doing the test again this morning with 3 concurrent requests: Once the first request is over, the 2 other (blocked) ones start at the same time. This is then where you might see an issue.

vicb · 2012-07-02T09:10:28Z

It seems that all storage handlers are affected by this issue.

The symptoms would not exactly be the same but let's say you have 2 concurrent requests. If they start at the same time they will both get the same copy of session data (as reading the session data would occur at about the same time).

Now let's say that one of the request results in updating / adding some values into the session and is the first to terminate, the updated / added value would be written into the session storage.

Then the second request terminates. The copy of the session it has has not been updated (it is the same at what the first request got while reading the initial data) but would overwrite the data updated / added by the other session.

Scenario:

request A and B arrives at about the same time, sessionA = sessionB = {foo: 1}
request A updates & adds some session values: sessionA = {foo: 2, bar: true}
request A terminates, the session is updated to {foo: 2, bar: true}
request B terminates, the session is updated to '{foo: 1}'
the changes made request A are lost

All of the storage handlers are concerned by this issue.

Native handlers are not concerned:

The native file handler locks files
The native memcached handler uses a lock
The native memcache handler also seems to work (tested with the attached gist)

@Drak what do you think ?

ghost · 2012-07-02T16:13:24Z

@vicb there is no harm in adding locking code although I am perplexed at the quoted C source which appears to be using memcached write itself to create the locks. I would like to see the return of the native driver classes.

@fabpot this particular PR is ready for merging imo

vicb · 2012-07-02T16:21:36Z

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

+        if (is_readable($path)) {
+            $handle = fopen($path, 'r');
+            flock($handle, LOCK_SH);
+            if (0 < $size = filesize($path)) {


why do we need filesize() here ? isn't the return value from fread() good enough - also filesize caches the file size.

If second argument in fread will be not greater than zero then PHP throw error.

The idea would be to issue a fread($handle, 50000) and loop if not at eof - you should really be careful with the filesize cache, got trapped a few times !

yanoosh · 2012-07-02T18:30:56Z

@vicb post realized me that my PR does not resolved problem with session handling. To get rid of all problem associated with sessions we have to decide:

add to storage handlers locks in open and close function
return to native handlers.

For me best solution will be 2 - in C code is condition for systems PHP_WIN32 https://github.com/php/php-src/blob/master/ext/session/mod_files.c#L172.

vicb · 2012-07-02T18:36:14Z

@yanoosh regarding your last message:

1- we must add locks to all session handlers,
2- we don't need "native handlers", native means handled by PHP, we don't need to handle it ourselves. The so called "native handlers" were only a mean to configure PHP (by modifying ini settings) - whether or not we need a mean of configuring PHP is out of topic here and at least has no reason to be done by the Session "component".

yanoosh · 2012-07-07T11:29:13Z

Implemented changes which are discussed. Waiting for your opinions.

ghost · 2012-07-08T11:03:51Z

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

+        return !(false === fwrite($this->handle, $data));
+    }
+
+    protected function initSessionFile($id){


The brace should be on a new line.

… replaced file_put/get_contents functions by fopen/fclose.

…nd removed else statement.

…ock in read and unlock in close method.

…nvoke method save and close to finish session.

… storage handler which is subject of test. In case when any of unrelated method will be damaged result of test case will be invalid.

ghost · 2012-07-08T14:20:14Z

Overall I think this PR is wrong to persist the session ID in the object instance - that's not how save handlers are supposed to operate - they don't track the session, they simply a CRUD driver - they should not be concerned with anything beyond doing that.

Before these recent changes, the PR was OK.

yanoosh · 2012-07-08T15:16:38Z

@Drak you have right that CRUD class should not track session, but how we prevent from overwrite session by other requests. Lock session must exist to properly handling a requests. We can move lock session somewhere else, but where ?

Commits ------- a351362 [HttpFoundation] Add NativeSessionHandler tests 653821a [HttpFoundation] Remove FileSessionHandler 3456787 Partially revert "[HttpFoundation][Sessions] Refactored tests" 39813a0 Revert "[FrameworkBundle] Refactor session file handler service name and update changelogs" fbee4cf Restore NativeFileSessionHandler Discussion ---------- [Session] Restore NativeFileSessionStorage Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: #4668 Todo: - License of the code: MIT This reverts the removal of the native file handler.

ooflorent reviewed Jun 28, 2012
View reviewed changes

ghost reviewed Jun 29, 2012
View reviewed changes

vicb reviewed Jul 2, 2012
View reviewed changes

ghost reviewed Jul 8, 2012
View reviewed changes

yanoosh added 6 commits July 8, 2012 13:17

[HttpFoundation][Sessions] Added flocks to read and write methods and…

dabedc1

… replaced file_put/get_contents functions by fopen/fclose.

[HttpFoundation][Sessions] Fixed code formatting.

a4dc42d

[HttpFoundation][Sessions] Fix. Added variable declaration above if a…

75d4890

…nd removed else statement.

[HttpFoundation][Sessions] Added locking session during a request - l…

a56d733

…ock in read and unlock in close method.

[HttpFoundation][Sessions] Fix. Function session_write_close always i…

cc841b5

…nvoke method save and close to finish session.

[HttpFoundation][Sessions] Test case should call only one method from…

0f242ac

… storage handler which is subject of test. In case when any of unrelated method will be damaged result of test case will be invalid.

[HttpFoundation][Sessions] Added test.

ce71c4e

ghost mentioned this pull request Jul 13, 2012

[Session] Restore NativeFileSessionStorage #4899

Merged

fabpot closed this Jul 13, 2012

vicb mentioned this pull request Jul 19, 2012

[HttpFoundation] Session handlers should use a lock #4976

Open

barryvdh mentioned this pull request Dec 30, 2014

Race condition on session engine causing unexpected behavior on concurrent requests laravel/framework#5416

Closed

Uh oh!

[HttpFoundation][Sessions] Losing session in case of many concurrent requests. #4668

[HttpFoundation][Sessions] Losing session in case of many concurrent requests. #4668

Uh oh!

Conversation

yanoosh commented Jun 26, 2012

Uh oh!

travisbot commented Jun 26, 2012

Uh oh!

vicb commented Jun 27, 2012

Uh oh!

ghost commented Jun 27, 2012

Uh oh!

vicb commented Jun 27, 2012

Uh oh!

ghost commented Jun 27, 2012

Uh oh!

yanoosh commented Jun 27, 2012

Uh oh!

vicb commented Jun 28, 2012

Uh oh!

ooflorent Jun 28, 2012

Choose a reason for hiding this comment

Uh oh!

travisbot commented Jun 28, 2012

Uh oh!

ghost Jun 29, 2012

Choose a reason for hiding this comment

Uh oh!

yanoosh commented Jun 29, 2012

Uh oh!

vicb commented Jul 2, 2012

Uh oh!

vicb commented Jul 2, 2012

Uh oh!

ghost commented Jul 2, 2012

Uh oh!

vicb Jul 2, 2012

Choose a reason for hiding this comment

Uh oh!

yanoosh Jul 2, 2012

Choose a reason for hiding this comment

Uh oh!

vicb Jul 2, 2012

Choose a reason for hiding this comment

Uh oh!

yanoosh commented Jul 2, 2012

Uh oh!

vicb commented Jul 2, 2012

Uh oh!

yanoosh commented Jul 7, 2012

Uh oh!

ghost Jul 8, 2012

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 8, 2012

Uh oh!

yanoosh commented Jul 8, 2012

Uh oh!

Uh oh!