Skip to content

Commit a076356

Browse files
fix(carddav): Handle race for SAB creation better
* Accept non repeatable read and INSERT conflict by another read * Handle replication edge case Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
1 parent 5cf42ff commit a076356

File tree

2 files changed

+29
-8
lines changed

2 files changed

+29
-8
lines changed

apps/dav/lib/CardDAV/CardDavBackend.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -389,6 +389,7 @@ public function updateAddressBook($addressBookId, \Sabre\DAV\PropPatch $propPatc
389389
* @param array $properties
390390
* @return int
391391
* @throws BadRequest
392+
* @throws Exception
392393
*/
393394
public function createAddressBook($principalUri, $url, array $properties) {
394395
if (strlen($url) > 255) {
@@ -433,7 +434,7 @@ public function createAddressBook($principalUri, $url, array $properties) {
433434
'synctoken' => $query->createParameter('synctoken'),
434435
])
435436
->setParameters($values)
436-
->execute();
437+
->executeStatement();
437438

438439
$addressBookId = $query->getLastInsertId();
439440
return [

apps/dav/lib/CardDAV/SyncService.php

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232

3333
use OCP\AppFramework\Db\TTransactional;
3434
use OCP\AppFramework\Http;
35+
use OCP\DB\Exception;
3536
use OCP\IDBConnection;
3637
use OCP\IUser;
3738
use OCP\IUserManager;
@@ -41,6 +42,7 @@
4142
use Sabre\DAV\Xml\Service;
4243
use Sabre\HTTP\ClientHttpException;
4344
use Sabre\VObject\Reader;
45+
use Throwable;
4446
use function is_null;
4547

4648
class SyncService {
@@ -116,15 +118,33 @@ public function syncRemoteAddressBook(string $url, string $userName, string $add
116118
* @throws \Sabre\DAV\Exception\BadRequest
117119
*/
118120
public function ensureSystemAddressBookExists(string $principal, string $uri, array $properties): ?array {
119-
return $this->atomic(function () use ($principal, $uri, $properties) {
120-
$book = $this->backend->getAddressBooksByUri($principal, $uri);
121-
if (!is_null($book)) {
122-
return $book;
121+
try {
122+
return $this->atomic(function () use ($principal, $uri, $properties) {
123+
$book = $this->backend->getAddressBooksByUri($principal, $uri);
124+
if (!is_null($book)) {
125+
return $book;
126+
}
127+
$this->backend->createAddressBook($principal, $uri, $properties);
128+
129+
return $this->backend->getAddressBooksByUri($principal, $uri);
130+
}, $this->dbConnection);
131+
} catch (Exception $e) {
132+
// READ COMMITTED doesn't prevent a nonrepeatable read above, so
133+
// two processes might create an address book here. Ignore our
134+
// failure and continue loading the entry written by the other process
135+
if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
136+
throw $e;
123137
}
124-
$this->backend->createAddressBook($principal, $uri, $properties);
125138

126-
return $this->backend->getAddressBooksByUri($principal, $uri);
127-
}, $this->dbConnection);
139+
// If this fails we might have hit a replication node that does not
140+
// have the row written in the other process.
141+
// TODO: find an elegant way to handle this
142+
$ab = $this->backend->getAddressBooksByUri($principal, $uri);
143+
if ($ab === null) {
144+
throw new Exception('Could not create system address book', $e->getCode(), $e);
145+
}
146+
return $ab;
147+
}
128148
}
129149

130150
/**

0 commit comments

Comments
 (0)