bug symfony#10652 [HttpFoundation] fix PDO session handler under high concurrency (Tobion)

This PR was merged into the 2.3 branch.

Discussion
----------

[HttpFoundation] fix PDO session handler under high concurrency

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#8448 and http://trac.symfony-project.org/ticket/4777 (which was never really fixed as you can see here)
| License       | MIT

- The first commit fixes PDO session handler under high concurrency.
- The second commit uses MERGE SQL for MS SQL Server. Tested with http://sqlfiddle.com/#!6/66b6d/14
- The third commit uses INSERT OR REPLACE for sqlite session handler http://sqlfiddle.com/#!7/e6707/3

What I find rather bad with the class design is that it depends on the table definition, but it's not part of the class. Also it doesn't make use of open() and close() which could be used to make the database connection lazy instead of having is open all the time when not needed. Doctrine also only lazy connects, but we use PDO directly here.
Furthermore, the session handlers should not throw exceptions, from what I read, but return false when an error occurs. This is not followed in this class. Maybe @Drak knows how php session management behaves when the session handlers return false?

Commits
-------

5c08e29 [HttpFoundation] use insert or replace for sqlite session handler
05ea19a [HttpFoundation] use MERGE SQL for MS SQL Server session storage
e58d7cf [HttpFoundation] fix PDO session handler under high concurrency

Author: fabpot

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

@@ -16,18 +16,34 @@
  *
  * @author Fabien Potencier <[email protected]>
  * @author Michael Williams <[email protected]>
+ * @author Tobias Schultze <http://tobion.de>
  */
 class PdoSessionHandler implements \SessionHandlerInterface
 {
     /**
-     * @var \PDO PDO instance.
+     * @var \PDO PDO instance
      */
     private $pdo;
 
     /**
-     * @var array Database options.
+     * @var string Table name
      */
-    private $dbOptions;
+    private $table;
+
+    /**
+     * @var string Column for session id
+     */
+    private $idCol;
+
+    /**
+     * @var string Column for session data
+     */
+    private $dataCol;
+
+    /**
+     * @var string Column for timestamp
+     */
+    private $timeCol;
 
     /**
      * Constructor.
@@ -52,11 +68,16 @@ public function __construct(\PDO $pdo, array $dbOptions = array())
             throw new \InvalidArgumentException(sprintf('"%s" requires PDO error mode attribute be set to throw Exceptions (i.e. $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION))', __CLASS__));
         }
         $this->pdo = $pdo;
-        $this->dbOptions = array_merge(array(
+        $dbOptions = array_merge(array(
             'db_id_col'   => 'sess_id',
             'db_data_col' => 'sess_data',
             'db_time_col' => 'sess_time',
         ), $dbOptions);
+
+        $this->table = $dbOptions['db_table'];
+        $this->idCol = $dbOptions['db_id_col'];
+        $this->dataCol = $dbOptions['db_data_col'];
+        $this->timeCol = $dbOptions['db_time_col'];
     }
 
     /**
@@ -80,19 +101,15 @@ public function close()
      */
     public function destroy($id)
     {
-        // get table/column
-        $dbTable = $this->dbOptions['db_table'];
-        $dbIdCol = $this->dbOptions['db_id_col'];
-
         // delete the record associated with this id
-        $sql = "DELETE FROM $dbTable WHERE $dbIdCol = :id";
+        $sql = "DELETE FROM $this->table WHERE $this->idCol = :id";
 
         try {
             $stmt = $this->pdo->prepare($sql);
             $stmt->bindParam(':id', $id, \PDO::PARAM_STR);
             $stmt->execute();
         } catch (\PDOException $e) {
-            throw new \RuntimeException(sprintf('PDOException was thrown when trying to manipulate session data: %s', $e->getMessage()), 0, $e);
+            throw new \RuntimeException(sprintf('PDOException was thrown when trying to delete a session: %s', $e->getMessage()), 0, $e);
         }
 
         return true;
@@ -103,19 +120,15 @@ public function destroy($id)
      */
     public function gc($lifetime)
     {
-        // get table/column
-        $dbTable   = $this->dbOptions['db_table'];
-        $dbTimeCol = $this->dbOptions['db_time_col'];
-
         // delete the session records that have expired
-        $sql = "DELETE FROM $dbTable WHERE $dbTimeCol < :time";
+        $sql = "DELETE FROM $this->table WHERE $this->timeCol < :time";
 
         try {
             $stmt = $this->pdo->prepare($sql);
             $stmt->bindValue(':time', time() - $lifetime, \PDO::PARAM_INT);
             $stmt->execute();
         } catch (\PDOException $e) {
-            throw new \RuntimeException(sprintf('PDOException was thrown when trying to manipulate session data: %s', $e->getMessage()), 0, $e);
+            throw new \RuntimeException(sprintf('PDOException was thrown when trying to delete expired sessions: %s', $e->getMessage()), 0, $e);
         }
 
         return true;
@@ -126,29 +139,20 @@ public function gc($lifetime)
      */
     public function read($id)
     {
-        // get table/columns
-        $dbTable   = $this->dbOptions['db_table'];
-        $dbDataCol = $this->dbOptions['db_data_col'];
-        $dbIdCol   = $this->dbOptions['db_id_col'];
+        $sql = "SELECT $this->dataCol FROM $this->table WHERE $this->idCol = :id";
 
         try {
-            $sql = "SELECT $dbDataCol FROM $dbTable WHERE $dbIdCol = :id";
-
             $stmt = $this->pdo->prepare($sql);
             $stmt->bindParam(':id', $id, \PDO::PARAM_STR);
-
             $stmt->execute();
-            // it is recommended to use fetchAll so that PDO can close the DB cursor
-            // we anyway expect either no rows, or one row with one column. fetchColumn, seems to be buggy #4777
+
+            // We use fetchAll instead of fetchColumn to make sure the DB cursor gets closed
             $sessionRows = $stmt->fetchAll(\PDO::FETCH_NUM);
 
-            if (count($sessionRows) == 1) {
+            if ($sessionRows) {
                 return base64_decode($sessionRows[0][0]);
             }
 
-            // session does not exist, create it
-            $this->createNewSession($id);
-
             return '';
         } catch (\PDOException $e) {
             throw new \RuntimeException(sprintf('PDOException was thrown when trying to read the session data: %s', $e->getMessage()), 0, $e);
@@ -160,84 +164,82 @@ public function read($id)
      */
     public function write($id, $data)
     {
-        // get table/column
-        $dbTable   = $this->dbOptions['db_table'];
-        $dbDataCol = $this->dbOptions['db_data_col'];
-        $dbIdCol   = $this->dbOptions['db_id_col'];
-        $dbTimeCol = $this->dbOptions['db_time_col'];
-
-        //session data can contain non binary safe characters so we need to encode it
+        // Session data can contain non binary safe characters so we need to encode it.
         $encoded = base64_encode($data);
 
+        // We use a MERGE SQL query when supported by the database.
+        // Otherwise we have to use a transactional DELETE followed by INSERT to prevent duplicate entries under high concurrency.
+
         try {
-            $driver = $this->pdo->getAttribute(\PDO::ATTR_DRIVER_NAME);
-
-            if ('mysql' === $driver) {
-                // MySQL would report $stmt->rowCount() = 0 on UPDATE when the data is left unchanged
-                // it could result in calling createNewSession() whereas the session already exists in
-                // the DB which would fail as the id is unique
-                $stmt = $this->pdo->prepare(
-                    "INSERT INTO $dbTable ($dbIdCol, $dbDataCol, $dbTimeCol) VALUES (:id, :data, :time) " .
-                    "ON DUPLICATE KEY UPDATE $dbDataCol = VALUES($dbDataCol), $dbTimeCol = VALUES($dbTimeCol)"
+            $mergeSql = $this->getMergeSql();
+
+            if (null !== $mergeSql) {
+                $mergeStmt = $this->pdo->prepare($mergeSql);
+                $mergeStmt->bindParam(':id', $id, \PDO::PARAM_STR);
+                $mergeStmt->bindParam(':data', $encoded, \PDO::PARAM_STR);
+                $mergeStmt->bindValue(':time', time(), \PDO::PARAM_INT);
+                $mergeStmt->execute();
+
+                return true;
+            }
+
+            $this->pdo->beginTransaction();
+
+            try {
+                $deleteStmt = $this->pdo->prepare(
+                    "DELETE FROM $this->table WHERE $this->idCol = :id"
+                );
+                $deleteStmt->bindParam(':id', $id, \PDO::PARAM_STR);
+                $deleteStmt->execute();
+
+                $insertStmt = $this->pdo->prepare(
+                    "INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time)"
                 );
-                $stmt->bindParam(':id', $id, \PDO::PARAM_STR);
-                $stmt->bindParam(':data', $encoded, \PDO::PARAM_STR);
-                $stmt->bindValue(':time', time(), \PDO::PARAM_INT);
-                $stmt->execute();
-            } elseif ('oci' === $driver) {
-                $stmt = $this->pdo->prepare("MERGE INTO $dbTable USING DUAL ON($dbIdCol = :id) ".
-                       "WHEN NOT MATCHED THEN INSERT ($dbIdCol, $dbDataCol, $dbTimeCol) VALUES (:id, :data, sysdate) " .
-                       "WHEN MATCHED THEN UPDATE SET $dbDataCol = :data WHERE $dbIdCol = :id");
-
-                $stmt->bindParam(':id', $id, \PDO::PARAM_STR);
-                $stmt->bindParam(':data', $encoded, \PDO::PARAM_STR);
-                $stmt->execute();
-            } else {
-                $stmt = $this->pdo->prepare("UPDATE $dbTable SET $dbDataCol = :data, $dbTimeCol = :time WHERE $dbIdCol = :id");
-                $stmt->bindParam(':id', $id, \PDO::PARAM_STR);
-                $stmt->bindParam(':data', $encoded, \PDO::PARAM_STR);
-                $stmt->bindValue(':time', time(), \PDO::PARAM_INT);
-                $stmt->execute();
-
-                if (!$stmt->rowCount()) {
-                    // No session exists in the database to update. This happens when we have called
-                    // session_regenerate_id()
-                    $this->createNewSession($id, $data);
-                }
+                $insertStmt->bindParam(':id', $id, \PDO::PARAM_STR);
+                $insertStmt->bindParam(':data', $encoded, \PDO::PARAM_STR);
+                $insertStmt->bindValue(':time', time(), \PDO::PARAM_INT);
+                $insertStmt->execute();
+
+                $this->pdo->commit();
+            } catch (\PDOException $e) {
+                $this->pdo->rollback();
+
+                throw $e;
             }
         } catch (\PDOException $e) {
-                throw new \RuntimeException(sprintf('PDOException was thrown when trying to write the session data: %s', $e->getMessage()), 0, $e);
+            throw new \RuntimeException(sprintf('PDOException was thrown when trying to write the session data: %s', $e->getMessage()), 0, $e);
         }
 
         return true;
     }
 
     /**
-     * Creates a new session with the given $id and $data
+     * Returns a merge/upsert (i.e. insert or update) SQL query when supported by the database.
      *
-     * @param string $id
-     * @param string $data
-     *
-     * @return boolean True.
+     * @return string|null The SQL string or null when not supported
      */
-    private function createNewSession($id, $data = '')
+    private function getMergeSql()
     {
-        // get table/column
-        $dbTable   = $this->dbOptions['db_table'];
-        $dbDataCol = $this->dbOptions['db_data_col'];
-        $dbIdCol   = $this->dbOptions['db_id_col'];
-        $dbTimeCol = $this->dbOptions['db_time_col'];
-
-        $sql = "INSERT INTO $dbTable ($dbIdCol, $dbDataCol, $dbTimeCol) VALUES (:id, :data, :time)";
-
-        //session data can contain non binary safe characters so we need to encode it
-        $encoded = base64_encode($data);
-        $stmt = $this->pdo->prepare($sql);
-        $stmt->bindParam(':id', $id, \PDO::PARAM_STR);
-        $stmt->bindParam(':data', $encoded, \PDO::PARAM_STR);
-        $stmt->bindValue(':time', time(), \PDO::PARAM_INT);
-        $stmt->execute();
+        $driver = $this->pdo->getAttribute(\PDO::ATTR_DRIVER_NAME);
+
+        switch ($driver) {
+            case 'mysql':
+                return "INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) " .
+                    "ON DUPLICATE KEY UPDATE $this->dataCol = VALUES($this->dataCol), $this->timeCol = VALUES($this->timeCol)";
+            case 'oci':
+                // DUAL is Oracle specific dummy table
+                return "MERGE INTO $this->table USING DUAL ON ($this->idCol = :id) " .
+                    "WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) " .
+                    "WHEN MATCHED THEN UPDATE SET $this->dataCol = :data";
+            case 'sqlsrv':
+                // MS SQL Server requires MERGE be terminated by semicolon
+                return "MERGE INTO $this->table USING (SELECT 'x' AS dummy) AS src ON ($this->idCol = :id) " .
+                    "WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) " .
+                    "WHEN MATCHED THEN UPDATE SET $this->dataCol = :data;";
+            case 'sqlite':
+                return "INSERT OR REPLACE INTO $this->table ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time)";
+        }
 
-        return true;
+        return null;
     }
 }