diff options
author | Christian Mollekopf <chrigi_1@fastmail.fm> | 2018-08-06 13:33:53 +0200 |
---|---|---|
committer | Christian Mollekopf <chrigi_1@fastmail.fm> | 2018-08-06 13:33:53 +0200 |
commit | f2fac63e086b4bd8dfc3ed9c9f31241eb9df9b9c (patch) | |
tree | eeaa0c8a560bf9c0b9814347353465f5dad36cd5 /tests | |
parent | b4341b9ba3f0242ff95fb229a80b4143c1369c34 (diff) | |
download | sink-f2fac63e086b4bd8dfc3ed9c9f31241eb9df9b9c.tar.gz sink-f2fac63e086b4bd8dfc3ed9c9f31241eb9df9b9c.zip |
Fixed threading issues with dynamic db creation.
This patch addresses two problems:
* A potential deadlock.
We had the following code inside a separately protected section:
dbiLocker.unlock();
//Here we could loos the readlock
QWriteLocker dbiWriteLocker(&sDbisLock);
If we lost the lock in between the two lines, the second thread that was
now holding a read-lock on sDbisLock could not enter the protected
section, which was a requirement to release the read-lock, and we'd thus
end up in a deadlock. This is solved using tryLock with intermediate
releases of the read-lock, allowing the original thread to finish.
* When failing to validate a dbi for the current transacation we
simply returned an invalid db (which then in this particular case broke
reading of revision uid's and type's), leading to queries not executing
as they should.
Both problems are unfortunately hard to reproduce, the adjusted test
at leaset allowed me to reproduce the deadlock situation sometimes.
To fix this cleanly we should probably just get rid of dynamic dbi
allocation for good.
Diffstat (limited to 'tests')
-rw-r--r-- | tests/storagetest.cpp | 8 |
1 files changed, 5 insertions, 3 deletions
diff --git a/tests/storagetest.cpp b/tests/storagetest.cpp index 4c44322..81acc13 100644 --- a/tests/storagetest.cpp +++ b/tests/storagetest.cpp | |||
@@ -191,14 +191,16 @@ private slots: | |||
191 | 191 | ||
192 | // We repeat the test a bunch of times since failing is relatively random | 192 | // We repeat the test a bunch of times since failing is relatively random |
193 | for (int tries = 0; tries < 10; tries++) { | 193 | for (int tries = 0; tries < 10; tries++) { |
194 | //clearEnv in combination with the bogus db layouts tests the dynamic named db opening as well. | ||
195 | Sink::Storage::DataStore::clearEnv(); | ||
194 | bool error = false; | 196 | bool error = false; |
195 | // Try to concurrently read | 197 | // Try to concurrently read |
196 | QList<QFuture<void>> futures; | 198 | QList<QFuture<void>> futures; |
197 | const int concurrencyLevel = 20; | 199 | const int concurrencyLevel = 20; |
198 | for (int num = 0; num < concurrencyLevel; num++) { | 200 | for (int num = 0; num < concurrencyLevel; num++) { |
199 | futures << QtConcurrent::run([this, &error]() { | 201 | futures << QtConcurrent::run([this, &error]() { |
200 | Sink::Storage::DataStore storage(testDataPath, dbName, Sink::Storage::DataStore::ReadOnly); | 202 | Sink::Storage::DataStore storage(testDataPath, {dbName, {{"bogus", 0}}}, Sink::Storage::DataStore::ReadOnly); |
201 | Sink::Storage::DataStore storage2(testDataPath, dbName + "2", Sink::Storage::DataStore::ReadOnly); | 203 | Sink::Storage::DataStore storage2(testDataPath, {dbName+ "2", {{"bogus", 0}}}, Sink::Storage::DataStore::ReadOnly); |
202 | for (int i = 0; i < count; i++) { | 204 | for (int i = 0; i < count; i++) { |
203 | if (!verify(storage, i)) { | 205 | if (!verify(storage, i)) { |
204 | error = true; | 206 | error = true; |
@@ -757,7 +759,7 @@ private slots: | |||
757 | 759 | ||
758 | Sink::Storage::DataStore::clearEnv(); | 760 | Sink::Storage::DataStore::clearEnv(); |
759 | //Try to read-write dynamic opening of the db. | 761 | //Try to read-write dynamic opening of the db. |
760 | //This is the case if we don't have all databases available upon initializatoin and we don't (e.g. because the db hasn't been created yet) | 762 | //This is the case if we don't have all databases available upon initialization and we don't (e.g. because the db hasn't been created yet) |
761 | { | 763 | { |
762 | // Trick the db into not loading all dbs by passing in a bogus layout. | 764 | // Trick the db into not loading all dbs by passing in a bogus layout. |
763 | Sink::Storage::DataStore store(testDataPath, {dbName, {{"bogus", 0}}}, Sink::Storage::DataStore::ReadWrite); | 765 | Sink::Storage::DataStore store(testDataPath, {dbName, {{"bogus", 0}}}, Sink::Storage::DataStore::ReadWrite); |