Skip to content

Commit

Permalink
Updates to FirestoreSessionHandler (#2360)
Browse files Browse the repository at this point in the history 
* calls open after running any Firestore close operation

* runs gc in separate transaction, sets max gcLimit to 500

* Fixes unit tests for FirestoreSessionHandler

* removes unnecessary try/catch blocks

* adds skipped test for session_gc bug

* small improvement
  • Loading branch information
@bshaffer @dwsupplee
bshaffer authored and dwsupplee committed Oct 2, 2019
1 parent e4174b8 commit be73255
Show file tree
Hide file tree
Showing 4 changed files with 158 additions and 76 deletions.
2 changes: 1 addition & 1 deletion Firestore/src/FirestoreClient.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ public function fieldPath(array $fieldNames)
* Configuration Options.
*
* @type int $gcLimit The number of entities to delete in the garbage
* collection. Values larger than 1000 will be limited to 1000.
* collection. Values larger than 500 will be limited to 500.
* **Defaults to** `0`, indicating garbage collection is disabled by
* default.
* @type string $collectionNameTemplate A sprintf compatible template
Expand Down
126 changes: 71 additions & 55 deletions Firestore/src/FirestoreSessionHandler.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
* $collectionNameTemplate option. This isolates the session data from your
* application data. It creates documents in the specified collection where the
* ID is the session ID. By default, it does nothing on gc for reducing the
* cost. Pass a positive value up to 1000 for $gcLimit option to delete entities
* cost. Pass a positive value up to 500 for $gcLimit option to delete entities
* in gc.
*
* The first example automatically writes the session data. It's handy, but
Expand Down Expand Up @@ -147,6 +147,10 @@ class FirestoreSessionHandler implements SessionHandlerInterface
* @var Transaction
*/
private $transaction;
/**
* @var string
*/
private $id;

/**
* Create a custom session handler backed by Cloud Firestore.
Expand All @@ -159,7 +163,7 @@ class FirestoreSessionHandler implements SessionHandlerInterface
* Configuration Options.
*
* @type int $gcLimit The number of entities to delete in the garbage
* collection. Values larger than 1000 will be limited to 1000.
* collection. Values larger than 500 will be limited to 500.
* **Defaults to** `0`, indicating garbage collection is disabled by
* default.
* @type string $collectionNameTemplate A sprintf compatible template
Expand Down Expand Up @@ -194,8 +198,8 @@ public function __construct(
'collectionNameTemplate' => '%1$s:%2$s',
];

// Cut down gcLimit to 1000
$this->options['gcLimit'] = min($this->options['gcLimit'], 1000);
// Cut down gcLimit to 500, as this is the Firestore batch limit.
$this->options['gcLimit'] = min($this->options['gcLimit'], 500);
}

/**
Expand Down Expand Up @@ -235,10 +239,22 @@ public function open($savePath, $sessionName)
}

/**
* Just return true for this implementation.
* Close the transaction and commit any changes.
*/
public function close()
{
if (is_null($this->transaction)) {
throw new \LogicException('open() must be called before close()');
}
try {
$this->commitTransaction($this->transaction);
} catch (ServiceException $e) {
trigger_error(
sprintf('Session close failed: %s', $e->getMessage()),
E_USER_WARNING
);
return false;
}
return true;
}

Expand All @@ -250,6 +266,7 @@ public function close()
*/
public function read($id)
{
$this->id = $id;
try {
$docRef = $this->getDocumentReference(
$this->connection,
Expand Down Expand Up @@ -283,26 +300,17 @@ public function read($id)
*/
public function write($id, $data)
{
try {
$docRef = $this->getDocumentReference(
$this->connection,
$this->valueMapper,
$this->projectId,
$this->database,
$this->docId($id)
);
$this->transaction->set($docRef, [
'data' => $data,
't' => time()
]);
$this->commitTransaction();
} catch (ServiceException $e) {
trigger_error(
sprintf('Firestore upsert failed: %s', $e->getMessage()),
E_USER_WARNING
);
return false;
}
$docRef = $this->getDocumentReference(
$this->connection,
$this->valueMapper,
$this->projectId,
$this->database,
$this->docId($id)
);
$this->transaction->set($docRef, [
'data' => $data,
't' => time()
]);
return true;
}

Expand All @@ -314,23 +322,14 @@ public function write($id, $data)
*/
public function destroy($id)
{
try {
$docRef = $this->getDocumentReference(
$this->connection,
$this->valueMapper,
$this->projectId,
$this->database,
$this->docId($id)
);
$this->transaction->delete($docRef);
$this->commitTransaction();
} catch (ServiceException $e) {
trigger_error(
sprintf('Firestore delete failed: %s', $e->getMessage()),
E_USER_WARNING
);
return false;
}
$docRef = $this->getDocumentReference(
$this->connection,
$this->valueMapper,
$this->projectId,
$this->database,
$this->docId($id)
);
$this->transaction->delete($docRef);
return true;
}

Expand All @@ -339,14 +338,27 @@ public function destroy($id)
*
* @param int $maxlifetime Remove all session data older than this number
* in seconds.
* @return bool
* @return int|bool
*/
public function gc($maxlifetime)
{
if (0 === $this->options['gcLimit']) {
return true;
}
$deleteCount = 0;
try {
$database = $this->databaseName($this->projectId, $this->database);
$beginTransaction = $this->connection->beginTransaction([
'database' => $database
] + $this->options['begin']);

$transaction = new Transaction(
$this->connection,
$this->valueMapper,
$database,
$beginTransaction['transaction']
);

$collectionRef = $this->getCollectionReference(
$this->connection,
$this->valueMapper,
Expand All @@ -358,41 +370,45 @@ public function gc($maxlifetime)
->limit($this->options['gcLimit'])
->where('t', '<', time() - $maxlifetime)
->orderBy('t');
$querySnapshot = $this->transaction->runQuery(
$querySnapshot = $transaction->runQuery(
$query,
$this->options['query']
);
foreach ($querySnapshot as $snapshot) {
$this->transaction->delete($snapshot->reference());
if ($snapshot->id() != $this->id) {
$transaction->delete($snapshot->reference());
$deleteCount++;
}
}
$this->commitTransaction();
$this->commitTransaction($transaction);
} catch (ServiceException $e) {
trigger_error(
sprintf('Session gc failed: %s', $e->getMessage()),
E_USER_WARNING
);
return false;
}
return true;
return $deleteCount;
}

/**
* Commit a transaction if changes exist, otherwise rollback the
* transaction. Also rollback if an exception is thrown.
*
* @throws \Exception
* @param Transaction $transaction The transaction to commit.
* @throws ServiceException
*/
private function commitTransaction()
private function commitTransaction(Transaction $transaction)
{
try {
if (!$this->transaction->writer()->isEmpty()) {
$this->transaction->writer()->commit($this->options['commit']);
if (!$transaction->writer()->isEmpty()) {
$transaction->writer()->commit($this->options['commit']);
} else {
// trigger rollback if no writes exist.
$this->transaction->writer()->rollback($this->options['rollback']);
$transaction->writer()->rollback($this->options['rollback']);
}
} catch (ServiceException $e) {
$this->transaction->writer()->rollback($this->options['rollback']);
$transaction->writer()->rollback($this->options['rollback']);

throw $e;
}
Expand All @@ -403,7 +419,7 @@ private function commitTransaction()
* name according to the $collectionNameTemplate option.
* ex: sessions:PHPSESSID
*
* @param string $id Identifier used for the session
* @param string $id Identifier used for the session.
* @return string
*/
private function collectionId()
Expand All @@ -419,7 +435,7 @@ private function collectionId()
* Format the Firebase document ID from the collection ID.
* ex: sessions:PHPSESSID/abcdef
*
* @param string $id Identifier used for the session
* @param string $id Identifier used for the session.
* @return string
*/
private function docId($id)
Expand Down
83 changes: 69 additions & 14 deletions Firestore/tests/System/FirestoreSessionHandlerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
*/
class FirestoreSessionHandlerTest extends FirestoreTestCase
{
public function testSessionHandler()
public function testSessionWrite()
{
$client = self::$client;

Expand All @@ -48,9 +48,9 @@ public function testSessionHandler()
sleep(1);

$hasDocument = false;
$query = $client->collection($namespace . ':' . session_name());
foreach ($query->documents() as $snapshot) {
self::$localDeletionQueue->add($snapshot->reference());
$collection = $client->collection($namespace . ':' . session_name());
self::$localDeletionQueue->add($collection);
foreach ($collection->documents() as $snapshot) {
if (!$hasDocument) {
$hasDocument = $snapshot['data'] === $storedValue;
}
Expand All @@ -59,25 +59,80 @@ public function testSessionHandler()
$this->assertTrue($hasDocument);
}

public function testSessionHandlerGarbageCollection()
public function testGarbageCollection()
{
$client = self::$client;

// Set session max lifetime to 0 to ensure deletion
ini_set('session.gc_maxlifetime', 0);

// Disable probability-based GC
ini_set('session.gc_probability', 0);

$namespace = uniqid('sess-' . self::COLLECTION_NAME);
$sessionName = 'PHPSESSID';
$collection = $client->collection($namespace . ':' . $sessionName);
$collection = $client->collection($namespace . ':' . session_name());
self::$localDeletionQueue->add($collection);
$collection->document('foo1')->set(['data' => 'foo1', 't' => time() - 1]);
$collection->document('foo2')->set(['data' => 'foo2', 't' => time() - 1]);

$this->assertCount(2, $collection->documents());
$collection->document('foo3')->set(['data' => 'foo3', 't' => time() + 1]);
$this->assertCount(3, $collection->documents());

$handler = $client->sessionHandler([
'gcLimit' => 1000,
'query' => ['maxRetries' => 0]
'gcLimit' => 500,
]);
$handler->open($namespace, $sessionName);
$handler->gc(0);

$this->assertCount(0, $collection->documents());
session_set_save_handler($handler, true);
session_save_path($namespace);
session_start();

session_gc();

$this->assertCount(1, $collection->documents());
}

public function testGarbageCollectionBeforeWrite()
{
$client = self::$client;

// Set session max lifetime to 0 to ensure deletion
ini_set('session.gc_maxlifetime', 0);

// Set GC divisor and probability to 1 so GC execution happens 100%
ini_set('session.gc_divisor', 1);
ini_set('session.gc_probability', 1);

$namespace = uniqid('sess-' . self::COLLECTION_NAME);
$content = 'foo';
$storedValue = 'name|' . serialize($content);
$collection = $client->collection($namespace . ':' . session_name());
self::$localDeletionQueue->add($collection);
$collection->document('foo1')->set(['data' => 'foo1', 't' => time() - 1]);
$collection->document('foo2')->set(['data' => 'foo2', 't' => time() - 1]);
$this->assertCount(2, $collection->documents());

$handler = $client->sessionHandler(['gcLimit' => 500]);
session_set_save_handler($handler, true);
session_save_path($namespace);
session_start();

$sessionId = session_id();
$_SESSION['name'] = $content;

session_write_close();
sleep(1);

// assert old records have been removed and the new record has been added.
$this->assertCount(1, $collection->documents());
}

public function testSessionGcReturnValue()
{
// "session_gc" returns false for user-defined session handlers.
// The following test will always fail:
// ```
// $this->assertGreaterThan(0, session_gc());
// ```
// This test is to remind us to implement a test the issue is fixed.
$this->markTestSkipped('session_gc returns false due to a core PHP bug');
}
}
Loading

0 comments on commit be73255

Please sign in to comment.