Skip to content

Commit 368f3c0

Browse files
trueqbittrueqbit
committed
Guarded connection lock against recursion
Simplified retaining connections for in-memory databases.
1 parent 987ff78 commit 368f3c0

File tree

3 files changed

+52
-42
lines changed

3 files changed

+52
-42
lines changed

‎dev/connection_holder.h‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,10 @@ namespace sqlite_orm {
2121
namespace internal {
2222

2323
#ifdef SQLITE_ORM_CPP20_SEMAPHORE_SUPPORTED
24-
/**
25-
* The connection holder should be performant in all variants:
24+
/*
25+
The connection holder should be performant in all variants:
2626
1. single-threaded use
27-
2. opened once (open forever)
27+
2. opened permanently (open forever)
2828
3. concurrent open/close
2929
3030
Hence, a light-weight binary semaphore is used to synchronize opening and closing a database connection.
@@ -33,19 +33,26 @@ namespace sqlite_orm {
3333
struct maybe_lock {
3434
maybe_lock(std::binary_semaphore& sync, bool shouldLock) noexcept(noexcept(sync.acquire())) :
3535
isSynced{shouldLock}, sync{sync} {
36-
if (shouldLock) {
37-
sync.acquire();
36+
if (isSynced) {
37+
if (++nRecursionsPerThread == 1) [[likely]] {
38+
sync.acquire();
39+
}
3840
}
3941
}
4042

4143
~maybe_lock() {
4244
if (isSynced) {
43-
sync.release();
45+
if (--nRecursionsPerThread == 0) [[likely]] {
46+
sync.release();
47+
}
4448
}
4549
}
4650

4751
const bool isSynced;
4852
std::binary_semaphore& sync;
53+
54+
// guard against recursive locking from the same thread in `on_open` callbacks
55+
inline static thread_local int nRecursionsPerThread = 0;
4956
};
5057

5158
connection_holder(std::string filename,
@@ -115,6 +122,10 @@ namespace sqlite_orm {
115122
return this->db;
116123
}
117124

125+
void propagate_open_forever_hint() {
126+
_openedForeverHint = true;
127+
}
128+
118129
/**
119130
* @attention While retrieving the reference count value is atomic it makes only sense at single-threaded points in code.
120131
*/
@@ -128,7 +139,7 @@ namespace sqlite_orm {
128139

129140
private:
130141
std::atomic_int _retainCount{};
131-
const bool _openedForeverHint = false;
142+
bool _openedForeverHint = false;
132143
std::binary_semaphore _sync{1};
133144

134145
private:
@@ -195,6 +206,8 @@ namespace sqlite_orm {
195206
return this->db;
196207
}
197208

209+
void propagate_open_forever_hint() {}
210+
198211
/**
199212
* @attention While retrieving the reference count value is atomic it makes only sense at single-threaded points in code.
200213
*/

‎dev/storage_base.h‎

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -294,12 +294,13 @@ namespace sqlite_orm {
294294
* needed and closes when it is not needed. This function establishes a permanent connection.
295295
* In-memory storage always establishes a permanent connection, so calling this method is a no-op.
296296
*
297-
* Attention: You must ensure that you cal lthis method in a single-threaded context.
297+
* Attention: You must ensure that to call this method only in a single-threaded context.
298298
* An alternative way to establish a permanent connection is to specify control options to `make_storage()`.
299299
*/
300300
void open_forever() {
301301
if (!this->isOpenedForever) {
302302
this->isOpenedForever = true;
303+
this->connection->propagate_open_forever_hint();
303304
this->connection->retain();
304305
}
305306
}
@@ -701,16 +702,14 @@ namespace sqlite_orm {
701702
int foreignKeysCount) :
702703
on_open{std::move(onOpenSpec.onOpen)}, pragma(std::bind(&storage_base::get_connection, this), executor),
703704
limit(std::bind(&storage_base::get_connection, this)),
704-
inMemory(filename.empty() || filename == ":memory:"), isOpenedForever{connectionCtrl.open_forever},
705+
inMemory(filename.empty() || filename == ":memory:"),
706+
isOpenedForever{connectionCtrl.open_forever || this->inMemory},
705707
connection(std::make_unique<connection_holder>(
706708
std::move(filename),
707709
std::bind(&storage_base::on_open_internal, this, std::placeholders::_1),
708710
connectionCtrl)),
709711
cachedForeignKeysCount(foreignKeysCount),
710712
executor{std::move(willRunQuerySpec.willRunQuery), std::move(didRunQuerySpec.didRunQuery)} {
711-
if (this->inMemory) {
712-
this->connection->retain();
713-
}
714713
if (this->isOpenedForever) {
715714
this->connection->retain();
716715
}
@@ -719,15 +718,12 @@ namespace sqlite_orm {
719718
storage_base(const storage_base& other) :
720719
on_open(other.on_open), pragma(std::bind(&storage_base::get_connection, this), executor),
721720
limit(std::bind(&storage_base::get_connection, this)), inMemory(other.inMemory),
722-
isOpenedForever{other.isOpenedForever},
721+
isOpenedForever{other.isOpenedForever || this->inMemory},
723722
connection(std::make_unique<connection_holder>(
724723
*other.connection,
725724
std::bind(&storage_base::on_open_internal, this, std::placeholders::_1))),
726725
cachedForeignKeysCount(other.cachedForeignKeysCount),
727726
executor{other.executor.will_run_query, other.executor.did_run_query} {
728-
if (this->inMemory) {
729-
this->connection->retain();
730-
}
731727
if (this->isOpenedForever) {
732728
this->connection->retain();
733729
}
@@ -737,9 +733,6 @@ namespace sqlite_orm {
737733
if (this->isOpenedForever) {
738734
this->connection->release();
739735
}
740-
if (this->inMemory) {
741-
this->connection->release();
742-
}
743736
}
744737

745738
void begin_transaction_internal(const std::string& sql) {
@@ -749,8 +742,7 @@ namespace sqlite_orm {
749742
}
750743

751744
connection_ref get_connection() {
752-
connection_ref res{*this->connection};
753-
return res;
745+
return {*this->connection};
754746
}
755747

756748
std::vector<std::string> object_names(string_constant_type type) {

‎include/sqlite_orm/sqlite_orm.h‎

Lines changed: 26 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -14648,10 +14648,10 @@ namespace sqlite_orm {
1464814648
namespace internal {
1464914649

1465014650
#ifdef SQLITE_ORM_CPP20_SEMAPHORE_SUPPORTED
14651-
/**
14652-
* The connection holder should be performant in all variants:
14651+
/*
14652+
The connection holder should be performant in all variants:
1465314653
1. single-threaded use
14654-
2. opened once (open forever)
14654+
2. opened permanently (open forever)
1465514655
3. concurrent open/close
1465614656

1465714657
Hence, a light-weight binary semaphore is used to synchronize opening and closing a database connection.
@@ -14660,19 +14660,26 @@ namespace sqlite_orm {
1466014660
struct maybe_lock {
1466114661
maybe_lock(std::binary_semaphore& sync, bool shouldLock) noexcept(noexcept(sync.acquire())) :
1466214662
isSynced{shouldLock}, sync{sync} {
14663-
if (shouldLock) {
14664-
sync.acquire();
14663+
if (isSynced) {
14664+
if (++nRecursionsPerThread == 1) [[likely]] {
14665+
sync.acquire();
14666+
}
1466514667
}
1466614668
}
1466714669

1466814670
~maybe_lock() {
1466914671
if (isSynced) {
14670-
sync.release();
14672+
if (--nRecursionsPerThread == 0) [[likely]] {
14673+
sync.release();
14674+
}
1467114675
}
1467214676
}
1467314677

1467414678
const bool isSynced;
1467514679
std::binary_semaphore& sync;
14680+
14681+
// guard against recursive locking from the same thread in `on_open` callbacks
14682+
inline static thread_local int nRecursionsPerThread = 0;
1467614683
};
1467714684

1467814685
connection_holder(std::string filename,
@@ -14742,6 +14749,10 @@ namespace sqlite_orm {
1474214749
return this->db;
1474314750
}
1474414751

14752+
void propagate_open_forever_hint() {
14753+
_openedForeverHint = true;
14754+
}
14755+
1474514756
/**
1474614757
* @attention While retrieving the reference count value is atomic it makes only sense at single-threaded points in code.
1474714758
*/
@@ -14755,7 +14766,7 @@ namespace sqlite_orm {
1475514766

1475614767
private:
1475714768
std::atomic_int _retainCount{};
14758-
const bool _openedForeverHint = false;
14769+
bool _openedForeverHint = false;
1475914770
std::binary_semaphore _sync{1};
1476014771

1476114772
private:
@@ -14822,6 +14833,8 @@ namespace sqlite_orm {
1482214833
return this->db;
1482314834
}
1482414835

14836+
void propagate_open_forever_hint() {}
14837+
1482514838
/**
1482614839
* @attention While retrieving the reference count value is atomic it makes only sense at single-threaded points in code.
1482714840
*/
@@ -19049,12 +19062,13 @@ namespace sqlite_orm {
1904919062
* needed and closes when it is not needed. This function establishes a permanent connection.
1905019063
* In-memory storage always establishes a permanent connection, so calling this method is a no-op.
1905119064
*
19052-
* Attention: You must ensure that you cal lthis method in a single-threaded context.
19065+
* Attention: You must ensure that to call this method only in a single-threaded context.
1905319066
* An alternative way to establish a permanent connection is to specify control options to `make_storage()`.
1905419067
*/
1905519068
void open_forever() {
1905619069
if (!this->isOpenedForever) {
1905719070
this->isOpenedForever = true;
19071+
this->connection->propagate_open_forever_hint();
1905819072
this->connection->retain();
1905919073
}
1906019074
}
@@ -19456,16 +19470,14 @@ namespace sqlite_orm {
1945619470
int foreignKeysCount) :
1945719471
on_open{std::move(onOpenSpec.onOpen)}, pragma(std::bind(&storage_base::get_connection, this), executor),
1945819472
limit(std::bind(&storage_base::get_connection, this)),
19459-
inMemory(filename.empty() || filename == ":memory:"), isOpenedForever{connectionCtrl.open_forever},
19473+
inMemory(filename.empty() || filename == ":memory:"),
19474+
isOpenedForever{connectionCtrl.open_forever || this->inMemory},
1946019475
connection(std::make_unique<connection_holder>(
1946119476
std::move(filename),
1946219477
std::bind(&storage_base::on_open_internal, this, std::placeholders::_1),
1946319478
connectionCtrl)),
1946419479
cachedForeignKeysCount(foreignKeysCount),
1946519480
executor{std::move(willRunQuerySpec.willRunQuery), std::move(didRunQuerySpec.didRunQuery)} {
19466-
if (this->inMemory) {
19467-
this->connection->retain();
19468-
}
1946919481
if (this->isOpenedForever) {
1947019482
this->connection->retain();
1947119483
}
@@ -19474,15 +19486,12 @@ namespace sqlite_orm {
1947419486
storage_base(const storage_base& other) :
1947519487
on_open(other.on_open), pragma(std::bind(&storage_base::get_connection, this), executor),
1947619488
limit(std::bind(&storage_base::get_connection, this)), inMemory(other.inMemory),
19477-
isOpenedForever{other.isOpenedForever},
19489+
isOpenedForever{other.isOpenedForever || this->inMemory},
1947819490
connection(std::make_unique<connection_holder>(
1947919491
*other.connection,
1948019492
std::bind(&storage_base::on_open_internal, this, std::placeholders::_1))),
1948119493
cachedForeignKeysCount(other.cachedForeignKeysCount),
1948219494
executor{other.executor.will_run_query, other.executor.did_run_query} {
19483-
if (this->inMemory) {
19484-
this->connection->retain();
19485-
}
1948619495
if (this->isOpenedForever) {
1948719496
this->connection->retain();
1948819497
}
@@ -19492,9 +19501,6 @@ namespace sqlite_orm {
1949219501
if (this->isOpenedForever) {
1949319502
this->connection->release();
1949419503
}
19495-
if (this->inMemory) {
19496-
this->connection->release();
19497-
}
1949819504
}
1949919505

1950019506
void begin_transaction_internal(const std::string& sql) {
@@ -19504,8 +19510,7 @@ namespace sqlite_orm {
1950419510
}
1950519511

1950619512
connection_ref get_connection() {
19507-
connection_ref res{*this->connection};
19508-
return res;
19513+
return {*this->connection};
1950919514
}
1951019515

1951119516
std::vector<std::string> object_names(string_constant_type type) {

0 commit comments

Comments
 (0)