Merge lp:~stolowski/unity-scope-click/fix-transaction-error into lp:unity-scope-click/devel
- fix-transaction-error
- Merge into devel
Status: | Merged |
---|---|
Approved by: | Alejandro J. Cura |
Approved revision: | 345 |
Merged at revision: | 341 |
Proposed branch: | lp:~stolowski/unity-scope-click/fix-transaction-error |
Merge into: | lp:unity-scope-click/devel |
Diff against target: |
330 lines (+193/-7) 3 files modified
libclickscope/click/departments-db.cpp (+32/-4) libclickscope/click/departments-db.h (+1/-1) libclickscope/tests/test_departments-db.cpp (+160/-2) |
To merge this branch: | bzr merge lp:~stolowski/unity-scope-click/fix-transaction-error |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
PS Jenkins bot (community) | continuous-integration | Approve | |
dobey (community) | Approve | ||
Alejandro J. Cura (community) | Approve | ||
Review via email: mp+227059@code.launchpad.net |
Commit message
Use write-ahead-log pragma of sqlite to fix transaction errors. Throw if transaction fails. Explicitly finish every query.
Description of the change
Use write-ahead-log pragma of sqlite to fix transaction errors. Throw if transaction fails. Explicitly finish every query.
PS Jenkins bot (ps-jenkins) wrote : | # |
dobey (dobey) wrote : | # |
Are there tests that go along with this code? Should they have tests to ensure that finish() is called in all the required cases, to avoid regression?
Alejandro J. Cura (alecu) wrote : | # |
Is there a way to add some kind of testing to this branch? Ideally, it would be a test that makes a transaction fail, and that gets fixed with this branch.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:340
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
Paweł Stołowski (stolowski) wrote : | # |
> Is there a way to add some kind of testing to this branch? Ideally, it would
> be a test that makes a transaction fail, and that gets fixed with this branch.
Ok, I've added two tests, one of them is a "stress test" that makes either reading or writing fail without WAL.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:343
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:345
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Alejandro J. Cura (alecu) wrote : | # |
Branch looks good, and the WAL fix seems reasonable.
The stress test is not so slow in my desktop machine (~5 seconds), but it might be due to the SSD.
In any case, I think we should consider moving that test into the integration tests if it gets slow for other developers.
dobey (dobey) : | # |
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
PS Jenkins bot (ps-jenkins) : | # |
Preview Diff
1 | === modified file 'libclickscope/click/departments-db.cpp' |
2 | --- libclickscope/click/departments-db.cpp 2014-07-10 08:57:59 +0000 |
3 | +++ libclickscope/click/departments-db.cpp 2014-07-17 11:09:40 +0000 |
4 | @@ -99,6 +99,11 @@ |
5 | // them for now. |
6 | // query.exec("PRAGMA foreign_keys = ON"); |
7 | |
8 | + // activate write-ahead logging, see http://sqlite.org/wal.html to avoid transaction errors with concurrent reads and writes |
9 | + query.exec("PRAGMA journal_mode=WAL"); |
10 | + |
11 | + db_.transaction(); |
12 | + |
13 | // package id -> department id mapping table |
14 | if (!query.exec("CREATE TABLE IF NOT EXISTS pkgmap (pkgid TEXT, deptid TEXT, CONSTRAINT pkey PRIMARY KEY (pkgid, deptid))")) |
15 | { |
16 | @@ -130,6 +135,11 @@ |
17 | { |
18 | report_db_error(query.lastError(), "Failed to create depts_v view"); |
19 | } |
20 | + |
21 | + if (!db_.commit()) |
22 | + { |
23 | + report_db_error(db_.lastError(), "Failed to commit init transaction"); |
24 | + } |
25 | } |
26 | |
27 | void DepartmentsDb::report_db_error(const QSqlError& error, const std::string& message) |
28 | @@ -151,9 +161,12 @@ |
29 | |
30 | if (select_dept_name_->next()) |
31 | { |
32 | - return select_dept_name_->value(0).toString().toStdString(); |
33 | + auto const res = select_dept_name_->value(0).toString().toStdString(); |
34 | + select_dept_name_->finish(); |
35 | + return res; |
36 | } |
37 | } |
38 | + select_dept_name_->finish(); |
39 | throw std::logic_error("No name for department " + department_id); |
40 | } |
41 | |
42 | @@ -166,9 +179,12 @@ |
43 | } |
44 | if (!select_parent_dept_->next()) |
45 | { |
46 | + select_dept_name_->finish(); |
47 | throw std::logic_error("Unknown department '" + department_id + "'"); |
48 | } |
49 | - return select_parent_dept_->value(0).toString().toStdString(); |
50 | + auto const res = select_parent_dept_->value(0).toString().toStdString(); |
51 | + select_parent_dept_->finish(); |
52 | + return res; |
53 | } |
54 | |
55 | std::list<DepartmentsDb::DepartmentInfo> DepartmentsDb::get_children_departments(const std::string& department_id) |
56 | @@ -187,6 +203,7 @@ |
57 | depts.push_back(inf); |
58 | } |
59 | |
60 | + select_children_depts_->finish(); |
61 | return depts; |
62 | } |
63 | |
64 | @@ -203,6 +220,7 @@ |
65 | { |
66 | pkgs.insert(query->value(0).toString().toStdString()); |
67 | } |
68 | + query->finish(); |
69 | return pkgs; |
70 | } |
71 | |
72 | @@ -226,6 +244,7 @@ |
73 | // delete package mapping first from any departments |
74 | delete_pkgmap_query_->bindValue(":pkgid", QVariant(QString::fromStdString(package_id))); |
75 | delete_pkgmap_query_->exec(); |
76 | + delete_pkgmap_query_->finish(); |
77 | |
78 | insert_pkgmap_query_->bindValue(":pkgid", QVariant(QString::fromStdString(package_id))); |
79 | insert_pkgmap_query_->bindValue(":deptid", QVariant(QString::fromStdString(department_id))); |
80 | @@ -238,9 +257,12 @@ |
81 | report_db_error(insert_pkgmap_query_->lastError(), "Failed to insert into pkgmap"); |
82 | } |
83 | |
84 | + insert_pkgmap_query_->finish(); |
85 | + |
86 | if (!db_.commit()) |
87 | { |
88 | - std::cerr << "Failed to commit transaction" << std::endl; |
89 | + db_.rollback(); |
90 | + report_db_error(db_.lastError(), "Failed to commit transaction in store_package_mapping"); |
91 | } |
92 | } |
93 | |
94 | @@ -262,6 +284,7 @@ |
95 | { |
96 | report_db_error(insert_dept_id_query_->lastError(), "Failed to insert into depts"); |
97 | } |
98 | + insert_dept_id_query_->finish(); |
99 | } |
100 | |
101 | void DepartmentsDb::store_department_name(const std::string& department_id, const std::string& locale, const std::string& name) |
102 | @@ -284,6 +307,7 @@ |
103 | { |
104 | report_db_error(insert_dept_name_query_->lastError(), "Failed to insert into deptnames"); |
105 | } |
106 | + insert_dept_name_query_->finish(); |
107 | } |
108 | |
109 | int DepartmentsDb::department_mapping_count() const |
110 | @@ -350,11 +374,15 @@ |
111 | report_db_error(delete_depts_query_->lastError(), "Failed to delete from depts"); |
112 | } |
113 | |
114 | + delete_deptnames_query_->finish(); |
115 | + delete_depts_query_->finish(); |
116 | + |
117 | store_departments_(depts, locale); |
118 | |
119 | if (!db_.commit()) |
120 | { |
121 | - std::cerr << "Failed to commit transaction" << std::endl; |
122 | + db_.rollback(); |
123 | + report_db_error(db_.lastError(), "Failed to commit transaction in store_departments"); |
124 | } |
125 | } |
126 | |
127 | |
128 | === modified file 'libclickscope/click/departments-db.h' |
129 | --- libclickscope/click/departments-db.h 2014-07-10 08:57:59 +0000 |
130 | +++ libclickscope/click/departments-db.h 2014-07-17 11:09:40 +0000 |
131 | @@ -82,7 +82,7 @@ |
132 | |
133 | static std::unique_ptr<DepartmentsDb> create_db(); |
134 | |
135 | -private: |
136 | +protected: |
137 | void init_db(const std::string& name); |
138 | void store_departments_(const click::DepartmentList& depts, const std::string& locale); |
139 | static void report_db_error(const QSqlError& error, const std::string& message); |
140 | |
141 | === modified file 'libclickscope/tests/test_departments-db.cpp' |
142 | --- libclickscope/tests/test_departments-db.cpp 2014-07-02 13:14:36 +0000 |
143 | +++ libclickscope/tests/test_departments-db.cpp 2014-07-17 11:09:40 +0000 |
144 | @@ -36,6 +36,30 @@ |
145 | |
146 | using namespace click; |
147 | |
148 | +class DepartmentsDbCheck : public DepartmentsDb |
149 | +{ |
150 | +public: |
151 | + DepartmentsDbCheck(const std::string& name) |
152 | + : DepartmentsDb(name) |
153 | + { |
154 | + } |
155 | + |
156 | + void check_sql_queries_finished() |
157 | + { |
158 | + EXPECT_FALSE(delete_pkgmap_query_->isActive()); |
159 | + EXPECT_FALSE(delete_depts_query_->isActive()); |
160 | + EXPECT_FALSE(delete_deptnames_query_->isActive()); |
161 | + EXPECT_FALSE(insert_pkgmap_query_->isActive()); |
162 | + EXPECT_FALSE(insert_dept_id_query_->isActive()); |
163 | + EXPECT_FALSE(insert_dept_name_query_->isActive()); |
164 | + EXPECT_FALSE(select_pkgs_by_dept_->isActive()); |
165 | + EXPECT_FALSE(select_pkgs_by_dept_recursive_->isActive()); |
166 | + EXPECT_FALSE(select_parent_dept_->isActive()); |
167 | + EXPECT_FALSE(select_children_depts_->isActive()); |
168 | + EXPECT_FALSE(select_dept_name_->isActive()); |
169 | + } |
170 | +}; |
171 | + |
172 | class DepartmentsDbTest: public ::testing::Test |
173 | { |
174 | public: |
175 | @@ -43,7 +67,7 @@ |
176 | |
177 | void SetUp() override |
178 | { |
179 | - db.reset(new DepartmentsDb(db_path)); |
180 | + db.reset(new DepartmentsDbCheck(db_path)); |
181 | db->store_department_name("tools", "", "Tools"); |
182 | db->store_department_name("office", "", "Office"); |
183 | |
184 | @@ -72,7 +96,18 @@ |
185 | } |
186 | |
187 | protected: |
188 | - std::unique_ptr<DepartmentsDb> db; |
189 | + std::unique_ptr<DepartmentsDbCheck> db; |
190 | +}; |
191 | + |
192 | +class DepartmentsDbConcurrencyTest: public ::testing::Test |
193 | +{ |
194 | +public: |
195 | + const std::string db_path = TEST_DIR "/departments-db-test2.sqlite"; |
196 | + |
197 | + void TearDown() override |
198 | + { |
199 | + unlink(db_path.c_str()); |
200 | + } |
201 | }; |
202 | |
203 | TEST_F(DepartmentsDbTest, testDepartmentNameLookup) |
204 | @@ -224,3 +259,126 @@ |
205 | EXPECT_EQ(4u, db->package_count()); |
206 | } |
207 | |
208 | +TEST_F(DepartmentsDbTest, testSqlQueriesFinished) |
209 | +{ |
210 | + db->get_department_name("games", {"en_EN", ""}); |
211 | + db->check_sql_queries_finished(); |
212 | + |
213 | + db->get_parent_department_id("rpg"); |
214 | + db->check_sql_queries_finished(); |
215 | + |
216 | + db->get_packages_for_department("rpg", false); |
217 | + db->check_sql_queries_finished(); |
218 | + |
219 | + db->get_packages_for_department("rpg", true); |
220 | + db->check_sql_queries_finished(); |
221 | + |
222 | + db->get_children_departments("games"); |
223 | + db->check_sql_queries_finished(); |
224 | + |
225 | + db->department_name_count(); |
226 | + db->check_sql_queries_finished(); |
227 | + |
228 | + db->department_mapping_count(); |
229 | + db->check_sql_queries_finished(); |
230 | + |
231 | + db->package_count(); |
232 | + db->check_sql_queries_finished(); |
233 | + |
234 | + db->store_package_mapping("game2", "fps"); |
235 | + db->check_sql_queries_finished(); |
236 | + |
237 | + db->store_department_name("games", "pl_PL", "Gry"); |
238 | + db->check_sql_queries_finished(); |
239 | + |
240 | + db->store_department_mapping("office", "tools"); |
241 | + db->check_sql_queries_finished(); |
242 | +} |
243 | + |
244 | +// Keep the numbers below at a reasonable level; it takes around 20 seconds |
245 | +// to run this test on a i7-2620M with the default values. |
246 | +const int NUM_OF_WRITE_OPS = 50; |
247 | +const int NUM_OF_READ_OPS = 100; |
248 | + |
249 | +void populate_departments(DepartmentsDb *db, int repeat_count) |
250 | +{ |
251 | + click::DepartmentList depts; |
252 | + // generate departments with one subdepartment |
253 | + for (int i = 0; i < 20; i++) |
254 | + { |
255 | + auto const id = std::to_string(i); |
256 | + auto parent = std::make_shared<click::Department>(id, "Department " + id, "href", true); |
257 | + parent->set_subdepartments({std::make_shared<click::Department>(id + "sub", "Subdepartment of " + id, "href", false)}); |
258 | + depts.push_back(parent); |
259 | + } |
260 | + |
261 | + for (int i = 0; i < repeat_count; i++) |
262 | + { |
263 | + ASSERT_NO_THROW(db->store_departments(depts, "")); |
264 | + |
265 | + // generate apps |
266 | + for (int j = 0; j < 50; j++) |
267 | + { |
268 | + auto const id = std::to_string(j); |
269 | + ASSERT_NO_THROW(db->store_package_mapping("app" + id, id)); |
270 | + } |
271 | + } |
272 | +} |
273 | + |
274 | +TEST_F(DepartmentsDbConcurrencyTest, ConcurrentReadWrite) |
275 | +{ |
276 | + // populate the db initially to make sure reader doesn't fail if it's faster than writer |
277 | + { |
278 | + DepartmentsDb db(db_path); |
279 | + populate_departments(&db, 1); |
280 | + } |
281 | + |
282 | + pid_t writer_pid = fork(); |
283 | + if (writer_pid < 0) |
284 | + { |
285 | + FAIL(); |
286 | + } |
287 | + else if (writer_pid == 0) // writer child process |
288 | + { |
289 | + SCOPED_TRACE("writer"); |
290 | + std::unique_ptr<DepartmentsDb> db; |
291 | + ASSERT_NO_THROW(db.reset(new DepartmentsDb(db_path))); |
292 | + |
293 | + populate_departments(db.get(), NUM_OF_WRITE_OPS); |
294 | + |
295 | + exit(0); |
296 | + } |
297 | + else // parent process |
298 | + { |
299 | + pid_t reader_pid = fork(); |
300 | + if (reader_pid < 0) |
301 | + { |
302 | + FAIL(); |
303 | + } |
304 | + else if (reader_pid == 0) // reader child process |
305 | + { |
306 | + SCOPED_TRACE("reader"); |
307 | + std::unique_ptr<DepartmentsDb> db; |
308 | + ASSERT_NO_THROW(db.reset(new DepartmentsDb(db_path))); |
309 | + |
310 | + for (int i = 0; i < NUM_OF_READ_OPS; i++) |
311 | + { |
312 | + ASSERT_NO_THROW(db->get_department_name("1", {""})); |
313 | + ASSERT_NO_THROW(db->get_parent_department_id("1")); |
314 | + ASSERT_NO_THROW(db->get_packages_for_department("1", false)); |
315 | + ASSERT_NO_THROW(db->get_packages_for_department("1", true)); |
316 | + ASSERT_NO_THROW(db->get_children_departments("")); |
317 | + ASSERT_NO_THROW(db->department_name_count()); |
318 | + ASSERT_NO_THROW(db->department_mapping_count()); |
319 | + ASSERT_NO_THROW(db->package_count()); |
320 | + } |
321 | + exit(0); |
322 | + } |
323 | + else // parent process |
324 | + { |
325 | + wait(nullptr); |
326 | + wait(nullptr); |
327 | + } |
328 | + } |
329 | +} |
330 | + |
PASSED: Continuous integration, rev:339 jenkins. qa.ubuntu. com/job/ unity-team- unity-scope- click-devel- ci/207/ jenkins. qa.ubuntu. com/job/ unity-team- unity-scope- click-devel- utopic- amd64-ci/ 182 jenkins. qa.ubuntu. com/job/ unity-team- unity-scope- click-devel- utopic- armhf-ci/ 181 jenkins. qa.ubuntu. com/job/ unity-team- unity-scope- click-devel- utopic- armhf-ci/ 181/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ unity-team- unity-scope- click-devel- utopic- i386-ci/ 181
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/unity- team-unity- scope-click- devel-ci/ 207/rebuild
http://