Skip to content

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

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.

Sign up for GitHub

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
wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dabedc1
[HttpFoundation][Sessions] Added flocks to read and write methods and…
yanoosh Jun 26, 2012
a4dc42d
[HttpFoundation][Sessions] Fixed code formatting.
yanoosh Jun 28, 2012
75d4890
[HttpFoundation][Sessions] Fix. Added variable declaration above if a…
yanoosh Jun 30, 2012
a56d733
[HttpFoundation][Sessions] Added locking session during a request - …
yanoosh Jul 5, 2012
cc841b5
[HttpFoundation][Sessions] Fix. Function session_write_close always i…
yanoosh Jul 6, 2012
0f242ac
[HttpFoundation][Sessions] Test case should call only one method from…
yanoosh Jul 6, 2012
ce71c4e
[HttpFoundation][Sessions] Added test.
yanoosh Jul 6, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 37 additions & 3 deletions src/Symfony/Component/HttpFoundation/Session/Storage/Handler/FileSessionHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ class FileSessionHandler implements \SessionHandlerInterface
*/
private $prefix;

/**
* @var resource
*/
private $handle = null;

/**
* @var string
*/
private $currentId = null;

/**
* Constructor.
*
Expand Down Expand Up @@ -61,6 +71,11 @@ public function open($savePath, $sessionName)
*/
public function close()
{
if (null !== $this->handle) {
flock($this->handle, LOCK_UN);
fclose($this->handle);
$this->currentId = null;
}
return true;
}

Expand All @@ -69,17 +84,36 @@ public function close()
*/
public function read($id)
{
$file = $this->getPath().$id;
$this->initSessionFile($id);
$data = '';
fseek($this->handle, 0);
while (!feof($this->handle)) {
$data .= fread($this->handle, 1048576);
}

return is_readable($file) ? file_get_contents($file) : '';
return $data;
}

/**
* {@inheritdoc}
*/
public function write($id, $data)
{
return false === file_put_contents($this->getPath().$id, $data) ? false : true;
$this->initSessionFile($id);
ftruncate($this->handle, 0);

return !(false === fwrite($this->handle, $data));
}

protected function initSessionFile($id)
{
if (null === $this->handle) {
$this->currentId = $id;
$this->handle = fopen($this->getPath() . $id, 'a+');
flock($this->handle, LOCK_EX);
} elseif ($id != $this->currentId) {
throw new \RuntimeException('You cannot manage two different sessions at the same time. Close current session before you start new one.');
Copy link

Choose a reason for hiding this comment

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

This condition will never occur because PHP will only open one session at a time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After session_start you can change session id. PHP allows for this type of behaviour and not throwing any error. I think the better solution will be inform user that he do something wrong trying to change session id.

Copy link

Choose a reason for hiding this comment

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

You cannot introduce behaviour than already exists in PHP sessions. Doing so will create inconsistencies across drivers. This is simply a storage device that is called by PHP itself and not invoked by user-land code and therefore we must and should not interfere. If you believe the behaviour of PHP sessions wrong, you would be quite correct, but that is a matter for PHP core.

}
}

/**
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/HttpFoundation/Session/Storage/MockFileSessionStorage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ public function regenerate($destroy = false, $lifetime = null)
public function save()
{
$this->handler->write($this->id, serialize($this->data));
$this->handler->close();

// this is needed for Silex, where the session object is re-used across requests
// in functional tests. In Symfony, the container is rebooted, so we don't have
Expand Down
33 changes: 24 additions & 9 deletions ...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/FileSessionHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,15 @@ class FileSessionStorageTest extends \PHPUnit_Framework_TestCase
*/
private $path;

/**
* @var string
*/
private $prefix = 'mocksess_';

public function setUp()
{
$this->path = sys_get_temp_dir().'/filesessionhandler';
$this->handler = new FileSessionHandler($this->path, 'mocksess_');
$this->handler = new FileSessionHandler($this->path, $this->prefix);

parent::setUp();
}
Expand Down Expand Up @@ -73,26 +78,27 @@ public function testReadWrite()

public function testDestroy()
{
$this->handler->write('456', 'data');
$pathPrefix = $this->path.'/'.$this->prefix;
file_put_contents($pathPrefix.'456', 'data');
$this->handler->destroy('123');
$this->assertEquals('data', $this->handler->read('456'));
$this->assertEquals('data', file_get_contents($pathPrefix . '456'));
$this->handler->destroy('456');
$this->assertEmpty($this->handler->read('456'));
$this->assertFalse(is_file($pathPrefix . '456'));
}

public function testGc()
{
$prefix = $this->path.'/mocksess_';
$this->handler->write('1', 'data');
$prefix = $this->path.'/'.$this->prefix;
file_put_contents($prefix.'1', 'data');
touch($prefix.'1', time()-86400);

$this->handler->write('2', 'data');
file_put_contents($prefix.'2', 'data');
touch($prefix.'2', time()-3600);

$this->handler->write('3', 'data');
file_put_contents($prefix.'3', 'data');
touch($prefix.'3', time()-300);

$this->handler->write('4', 'data');
file_put_contents($prefix.'4', 'data');

$this->handler->gc(90000);
$this->assertEquals(4, count(glob($this->path.'/*')));
Expand All @@ -103,4 +109,13 @@ public function testGc()
$this->handler->gc(200);
$this->assertEquals(1, count(glob($this->path.'/*')));
}

/**
* @expectedException RuntimeException
*/
public function testOneSessionAtTime()
Copy link

Choose a reason for hiding this comment

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

This test is pointless. PHP handles this by itself.

{
$this->handler->read(1);
$this->handler->read(2);
}
}