Skip to content

Commit 44b9d84

Browse files
fabpotfabpot
committed
feature #11068 [HttpFoundation] #11009 for master (Tobion)
This PR was merged into the 2.3-dev branch. Discussion ---------- [HttpFoundation] #11009 for master | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT Commits ------- 8d375ca [HttpFoundation] merge #11009 into master
2 parents 97750f7 + 8d375ca commit 44b9d84

File tree

4 files changed

+83
-56
lines changed

4 files changed

+83
-56
lines changed

‎src/Symfony/Bridge/Doctrine/HttpFoundation/DbalSessionHandler.php

+44-31
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
namespace Symfony\Bridge\Doctrine\HttpFoundation;
1313

1414
use Doctrine\DBAL\Connection;
15+
use Doctrine\DBAL\Driver\DriverException;
16+
use Doctrine\DBAL\Platforms\SQLServer2008Platform;
1517

1618
/**
1719
* DBAL based session storage.
@@ -146,13 +148,10 @@ public function read($sessionId)
146148
*/
147149
public function write($sessionId, $data)
148150
{
149-
// Session data can contain non binary safe characters so we need to encode it.
150151
$encoded = base64_encode($data);
151152

152-
// We use a MERGE SQL query when supported by the database.
153-
// Otherwise we have to use a transactional DELETE followed by INSERT to prevent duplicate entries under high concurrency.
154-
155153
try {
154+
// We use a single MERGE SQL query when supported by the database.
156155
$mergeSql = $this->getMergeSql();
157156

158157
if (null !== $mergeSql) {
@@ -165,28 +164,41 @@ public function write($sessionId, $data)
165164
return true;
166165
}
167166

168-
$this->con->beginTransaction();
169-
170-
try {
171-
$deleteStmt = $this->con->prepare(
172-
"DELETE FROM $this->table WHERE $this->idCol = :id"
173-
);
174-
$deleteStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR);
175-
$deleteStmt->execute();
176-
177-
$insertStmt = $this->con->prepare(
178-
"INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time)"
179-
);
180-
$insertStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR);
181-
$insertStmt->bindParam(':data', $encoded, \PDO::PARAM_STR);
182-
$insertStmt->bindValue(':time', time(), \PDO::PARAM_INT);
183-
$insertStmt->execute();
184-
185-
$this->con->commit();
186-
} catch (\Exception $e) {
187-
$this->con->rollback();
188-
189-
throw $e;
167+
$updateStmt = $this->con->prepare(
168+
"UPDATE $this->table SET $this->dataCol = :data, $this->timeCol = :time WHERE $this->idCol = :id"
169+
);
170+
$updateStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR);
171+
$updateStmt->bindParam(':data', $encoded, \PDO::PARAM_STR);
172+
$updateStmt->bindValue(':time', time(), \PDO::PARAM_INT);
173+
$updateStmt->execute();
174+
175+
// When MERGE is not supported, like in Postgres, we have to use this approach that can result in
176+
// duplicate key errors when the same session is written simultaneously. We can just catch such an
177+
// error and re-execute the update. This is similar to a serializable transaction with retry logic
178+
// on serialization failures but without the overhead and without possible false positives due to
179+
// longer gap locking.
180+
if (!$updateStmt->rowCount()) {
181+
try {
182+
$insertStmt = $this->con->prepare(
183+
"INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time)"
184+
);
185+
$insertStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR);
186+
$insertStmt->bindParam(':data', $encoded, \PDO::PARAM_STR);
187+
$insertStmt->bindValue(':time', time(), \PDO::PARAM_INT);
188+
$insertStmt->execute();
189+
} catch (\Exception $e) {
190+
$driverException = $e->getPrevious();
191+
// Handle integrity violation SQLSTATE 23000 (or a subclass like 23505 in Postgres) for duplicate keys
192+
// DriverException only available since DBAL 2.5
193+
if (
194+
($driverException instanceof DriverException && 0 === strpos($driverException->getSQLState(), '23')) ||
195+
($driverException instanceof \PDOException && 0 === strpos($driverException->getCode(), '23'))
196+
) {
197+
$updateStmt->execute();
198+
} else {
199+
throw $e;
200+
}
201+
}
190202
}
191203
} catch (\Exception $e) {
192204
throw new \RuntimeException(sprintf('Exception was thrown when trying to write the session data: %s', $e->getMessage()), 0, $e);
@@ -212,12 +224,13 @@ private function getMergeSql()
212224
// DUAL is Oracle specific dummy table
213225
return "MERGE INTO $this->table USING DUAL ON ($this->idCol = :id) " .
214226
"WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) " .
215-
"WHEN MATCHED THEN UPDATE SET $this->dataCol = :data";
216-
case 'mssql':
217-
// MS SQL Server requires MERGE be terminated by semicolon
218-
return "MERGE INTO $this->table USING (SELECT 'x' AS dummy) AS src ON ($this->idCol = :id) " .
227+
"WHEN MATCHED THEN UPDATE SET $this->dataCol = :data, $this->timeCol = :time";
228+
case $this->con->getDatabasePlatform() instanceof SQLServer2008Platform:
229+
// MERGE is only available since SQL Server 2008 and must be terminated by semicolon
230+
// It also requires HOLDLOCK according to http://weblogs.sqlteam.com/dang/archive/2009/01/31/UPSERT-Race-Condition-With-MERGE.aspx
231+
return "MERGE INTO $this->table WITH (HOLDLOCK) USING (SELECT 1 AS dummy) AS src ON ($this->idCol = :id) " .
219232
"WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) " .
220-
"WHEN MATCHED THEN UPDATE SET $this->dataCol = :data;";
233+
"WHEN MATCHED THEN UPDATE SET $this->dataCol = :data, $this->timeCol = :time;";
221234
case 'sqlite':
222235
return "INSERT OR REPLACE INTO $this->table ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time)";
223236
}

‎src/Symfony/Bridge/Doctrine/HttpFoundation/DbalSessionHandlerSchema.php

+3-6
Original file line numberDiff line numberDiff line change
@@ -20,14 +20,11 @@
2020
*/
2121
final class DbalSessionHandlerSchema extends Schema
2222
{
23-
private $tableName;
24-
2523
public function __construct($tableName = 'sessions')
2624
{
2725
parent::__construct();
2826

29-
$this->tableName = $tableName;
30-
$this->addSessionTable();
27+
$this->addSessionTable($tableName);
3128
}
3229

3330
public function addToSchema(Schema $schema)
@@ -37,9 +34,9 @@ public function addToSchema(Schema $schema)
3734
}
3835
}
3936

40-
private function addSessionTable()
37+
private function addSessionTable($tableName)
4138
{
42-
$table = $this->createTable($this->tableName);
39+
$table = $this->createTable($tableName);
4340
$table->addColumn('sess_id', 'string');
4441
$table->addColumn('sess_data', 'text')->setNotNull(true);
4542
$table->addColumn('sess_time', 'integer')->setNotNull(true)->setUnsigned(true);

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

+34-17
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@
1919
* This means requests for the same session will wait until the other one finished.
2020
* PHPs internal files session handler also works this way.
2121
*
22+
* Session data is a binary string that can contain non-printable characters like the null byte.
23+
+ For this reason this handler base64 encodes the data to be able to save it in a character column.
24+
*
2225
* Attention: Since SQLite does not support row level locks but locks the whole database,
2326
* it means only one session can be accessed at a time. Even different sessions would wait
2427
* for another to finish. So saving session in SQLite should only be considered for
@@ -192,13 +195,12 @@ public function destroy($sessionId)
192195
*/
193196
public function write($sessionId, $data)
194197
{
195-
// Session data can contain non binary safe characters so we need to encode it.
196198
$encoded = base64_encode($data);
197199

198200
// The session ID can be different from the one previously received in read()
199201
// when the session ID changed due to session_regenerate_id(). So we have to
200202
// do an insert or update even if we created a row in read() for locking.
201-
// We use a MERGE SQL query when supported by the database.
203+
// We use a single MERGE SQL query when supported by the database.
202204

203205
try {
204206
$mergeSql = $this->getMergeSql();
@@ -221,17 +223,30 @@ public function write($sessionId, $data)
221223
$updateStmt->bindValue(':time', time(), \PDO::PARAM_INT);
222224
$updateStmt->execute();
223225

224-
// Since we have a lock on the session, this is safe to do. Otherwise it would be prone to
225-
// race conditions in high concurrency. And if it's a regenerated session ID it should be
226-
// unique anyway.
226+
// When MERGE is not supported, like in Postgres, we have to use this approach that can result in
227+
// duplicate key errors when the same session is written simultaneously. We can just catch such an
228+
// error and re-execute the update. This is similar to a serializable transaction with retry logic
229+
// on serialization failures but without the overhead and without possible false positives due to
230+
// longer gap locking.
231+
// Since we have a lock on the session, the above case should not happen. And if it's a regenerated
232+
// session ID it should be unique anyway.
227233
if (!$updateStmt->rowCount()) {
228-
$insertStmt = $this->pdo->prepare(
229-
"INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time)"
230-
);
231-
$insertStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR);
232-
$insertStmt->bindParam(':data', $encoded, \PDO::PARAM_STR);
233-
$insertStmt->bindValue(':time', time(), \PDO::PARAM_INT);
234-
$insertStmt->execute();
234+
try {
235+
$insertStmt = $this->pdo->prepare(
236+
"INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time)"
237+
);
238+
$insertStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR);
239+
$insertStmt->bindParam(':data', $encoded, \PDO::PARAM_STR);
240+
$insertStmt->bindValue(':time', time(), \PDO::PARAM_INT);
241+
$insertStmt->execute();
242+
} catch (\PDOException $e) {
243+
// Handle integrity violation SQLSTATE 23000 (or a subclass like 23505 in Postgres) for duplicate keys
244+
if (0 === strpos($e->getCode(), '23')) {
245+
$updateStmt->execute();
246+
} else {
247+
throw $e;
248+
}
249+
}
235250
}
236251
} catch (\PDOException $e) {
237252
$this->rollback();
@@ -353,9 +368,10 @@ private function lockSession($sessionId)
353368
"WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) " .
354369
"WHEN MATCHED THEN UPDATE SET $this->idCol = $this->idCol";
355370
break;
356-
case 'sqlsrv':
371+
// todo: implement locking for SQL Server < 2008
372+
case 'sqlsrv' === $this->driver && version_compare($this->pdo->getAttribute(\PDO::ATTR_SERVER_VERSION), '10', '>='):
357373
// MS SQL Server requires MERGE be terminated by semicolon
358-
$sql = "MERGE INTO $this->table USING (SELECT 'x' AS dummy) AS src ON ($this->idCol = :id) " .
374+
$sql = "MERGE INTO $this->table WITH (HOLDLOCK) USING (SELECT 1 AS dummy) AS src ON ($this->idCol = :id) " .
359375
"WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) " .
360376
"WHEN MATCHED THEN UPDATE SET $this->idCol = $this->idCol;";
361377
break;
@@ -397,9 +413,10 @@ private function getMergeSql()
397413
return "MERGE INTO $this->table USING DUAL ON ($this->idCol = :id) " .
398414
"WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) " .
399415
"WHEN MATCHED THEN UPDATE SET $this->dataCol = :data, $this->timeCol = :time";
400-
case 'sqlsrv':
401-
// MS SQL Server requires MERGE be terminated by semicolon
402-
return "MERGE INTO $this->table USING (SELECT 'x' AS dummy) AS src ON ($this->idCol = :id) " .
416+
case 'sqlsrv' === $this->driver && version_compare($this->pdo->getAttribute(\PDO::ATTR_SERVER_VERSION), '10', '>='):
417+
// MERGE is only available since SQL Server 2008 and must be terminated by semicolon
418+
// It also requires HOLDLOCK according to http://weblogs.sqlteam.com/dang/archive/2009/01/31/UPSERT-Race-Condition-With-MERGE.aspx
419+
return "MERGE INTO $this->table WITH (HOLDLOCK) USING (SELECT 1 AS dummy) AS src ON ($this->idCol = :id) " .
403420
"WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) " .
404421
"WHEN MATCHED THEN UPDATE SET $this->dataCol = :data, $this->timeCol = :time;";
405422
case 'sqlite':

‎src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php

+2-2
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,9 @@ protected function setUp()
2323
$this->markTestSkipped('This test requires SQLite support in your environment');
2424
}
2525

26-
$this->pdo = new \PDO("sqlite::memory:");
26+
$this->pdo = new \PDO('sqlite::memory:');
2727
$this->pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);
28-
$sql = "CREATE TABLE sessions (sess_id VARCHAR(255) PRIMARY KEY, sess_data TEXT, sess_time INTEGER)";
28+
$sql = 'CREATE TABLE sessions (sess_id VARCHAR(128) PRIMARY KEY, sess_data TEXT, sess_time INTEGER)';
2929
$this->pdo->exec($sql);
3030
}
3131

0 commit comments

Comments
 (0)