Closed Bug 1036440 Opened 10 years ago Closed 8 years ago

Rewriting PlacesDBUtils using async/await and promiseDBConnection

Categories

(Toolkit :: Places, defect, P2)

Product:
Toolkit
Component:
Places
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ahameez, Assigned: milindl, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1036440 - Refactor and use async/await and withConnectionWrapper/promiseDBConnection in PlacesDBUtils,
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1036440 - Fix tests and consumers for rewritten `PlacesDBUtils`,
59 bytes, text/x-review-board-request
mak
: review+
Details
Rewriting PlacesDBUtils using Tasks.jsm and promiseDBConnection
Assignee: nobody → althaf.mozilla
Status: NEW → ASSIGNED
Mentor: mak77
Priority: -- → P2
Assignee: althaf.mozilla → nobody
Status: ASSIGNED → NEW
I've submitted a patch for this. Please tell me if I am going about this the right way. I have a few issues, firstly, I'm not sure to do with `handleError` calls - I've removed most of them - since `executeCached` is a promise which rejects (I catch it and deal with it). I think I also need to fix up all the test files found by searchfox.org/mozilla-central/search?q=PlacesDBUtils as well as the file PlacesCategoriesStarter.js (do I make it async as well?). Some of the tests are not using add_tasks, they use run_test, so I will also fix that Some doubts are inside the file itself. Thanks!
(In reply to Milind L (:milindl) from comment #5) > Comment on attachment 8867521 [details] > Bug 1036440 - use async/await and withConnectionWrapper/promiseDBConnection > to rewrite PlacesDBUtils, > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/139070/diff/3-4/ I tested a bit today and with this diff, I have some good news: all the tests in http://searchfox.org/mozilla-central/search?q=PlacesDBUtils pass locally without any modification to the code in the tests themselves :) Some of the tests use a mechanism of waiting on a notification sent out by maintenenceOnIdle instead of maybe awaiting the function, however, since we still send out that notification I've kept that intact. Tell me if we need to change this approach or are the tests fine as they are. I am still unsure about PlacesCategoriesStarter.js, I am not sure how to refactor that. Thanks!
(In reply to Milind L (:milindl) from comment #2) > I've submitted a patch for this. Please tell me if I am going about this the > right way. I have a few issues, firstly, I'm not sure to do with > `handleError` calls - I've removed most of them - since `executeCached` is a > promise which rejects (I catch it and deal with it). > > I think I also need to fix up all the test files found by > searchfox.org/mozilla-central/search?q=PlacesDBUtils as well as the file > PlacesCategoriesStarter.js (do I make it async as well?). it should probably just .catch(Cu.reportError); the maintenanceOnIdle() promise.
Comment on attachment 8867521 [details] Bug 1036440 - Refactor and use async/await and withConnectionWrapper/promiseDBConnection in PlacesDBUtils, https://reviewboard.mozilla.org/r/139070/#review144058 ::: toolkit/components/places/PlacesDBUtils.jsm:14 (Diff revision 5) > const Cr = Components.results; > const Cu = Components.utils; > > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > -Cu.import("resource://gre/modules/Services.jsm"); > -Cu.import("resource://gre/modules/PlacesUtils.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "Services", > + "resource://gre/modules/Services.jsm"); not worth lazifying Services, it's used everywhere like XPCOMUtils. ::: toolkit/components/places/PlacesDBUtils.jsm:35 (Diff revision 5) > * FINISHED_MAINTENANCE_TOPIC is notified through observer service on finish. > * > * @param aTasks > * Tasks object to execute. > */ > - _executeTasks: function PDBU__executeTasks(aTasks) { > + async _executeTasks(aTasks) { So, I'd like to clarify one thing, the code here is horrible, I wrote this when we didn't have Task, async and promises. Due to that, I'm happy if you want to also refactor some of it. For example, this _executeTasks method takes a list of tags, executs them, and for async tasks it waits and then calls back itself. That's horrible. With promises and async we could probably make this just run a simple loop and wait for every promise, without having to pop a task and call back to ourselves. ::: toolkit/components/places/PlacesDBUtils.jsm:130 (Diff revision 5) > - _refreshUI: function PDBU__refreshUI(aTasks) { > + async _refreshUI(aTasks) { > let tasks = new Tasks(aTasks); > > // Send batch update notifications to update the UI. > + // DOUBT: can we replace this with simply sending out the notifications somehow, > + // instead of calling the old API and faking a batch mode to send notifications? Yes, we can use history.getObservers() and directly notify them onBeginUpdateBatch() and onEndUpdateBatch() in a loop. ::: toolkit/components/places/PlacesDBUtils.jsm:183 (Diff revision 5) > - checkIntegrity: function PDBU_checkIntegrity(aTasks, aSkipReindex) { > + async checkIntegrity(aTasks, aSkipReindex) { > let tasks = new Tasks(aTasks); > tasks.log("> Integrity check"); > > // Run a integrity check, but stop at the first error. > - let stmt = DBConn.createAsyncStatement("PRAGMA integrity_check(1)"); > + // DOUBT: I'm not sure how to stop at the first error when using `row` callback throw StopIteration; it should just stop returning results and resolve. It's horrible but we didn't find a better way for now. ::: toolkit/components/places/PlacesDBUtils.jsm:228 (Diff revision 5) > tasks.log("> Coherence check"); > - > - let stmts = PlacesDBUtils._getBoundCoherenceStatements(); > - DBConn.executeAsync(stmts, stmts.length, { > - handleError: PlacesDBUtils._handleError, > - handleResult() {}, > + // Statements can be executed asynchronously. > + let stmts = await PlacesDBUtils._getBoundCoherenceStatements(); > + let allStatementsPromises = []; > + let coherenceCheck = true; > + stmts.forEach(({query, params}) => { for...of is faster than .forEach, so it should in general be preferred ::: toolkit/components/places/PlacesDBUtils.jsm:239 (Diff revision 5) > + coherenceCheck = false; > + }); > + allStatementsPromises.push(promise); > + }); > + }); > + await Promise.all(allStatementsPromises); I'm wondering whether we want to use Promise.all here or not. The problem with Promise.all is that it will stop at the first rejection. The previous code continued instead. Maybe you should just await and try/catch in the for loop to stay consistent on how things were before. ::: toolkit/components/places/PlacesDBUtils.jsm:814 (Diff revision 5) > - expire: function PDBU_expire(aTasks) { > + async expire(aTasks) { > let tasks = new Tasks(aTasks); > tasks.log("> Orphans expiration"); > > let expiration = Cc["@mozilla.org/places/expiration;1"]. > - getService(Ci.nsIObserver); > + getService(Ci.nsIObserver); we rather align dots and parenthesis like: let expiration = Cc["@mozilla.org/places/expiration;1"] .getService(Ci.nsIObserver); ::: toolkit/components/places/PlacesDBUtils.jsm:816 (Diff revision 5) > tasks.log("> Orphans expiration"); > > let expiration = Cc["@mozilla.org/places/expiration;1"]. > - getService(Ci.nsIObserver); > + getService(Ci.nsIObserver); > > - Services.obs.addObserver(function(aSubject, aTopic, aData) { > + Services.obs.addObserver(async function(aSubject, aTopic, aData) { I'm not sure this works, addObserver accepts normal functions, you may probably do this inside the normal function instead: (async function() { ... })(); ::: toolkit/components/places/PlacesDBUtils.jsm:849 (Diff revision 5) > - , "cache_size" > + , "cache_size" > - , "journal_mode" > + , "journal_mode" > - , "synchronous" > + , "synchronous" > - ].forEach(function(aPragma) { > - let stmt = DBConn.createStatement("PRAGMA " + aPragma); > - stmt.executeStep(); > + ].map((aPragma) => { > + return PlacesUtils.withConnectionWrapper( > + "PlacesDBUtils: pragma for stats", we're lucky, Sqlite added recently support for pragma functions, so now we can do this in a single query: SELECT * FROM pragma_user_version, pragma_page_size, ... will return one column per pragma. ::: toolkit/components/places/PlacesDBUtils.jsm:943 (Diff revision 5) > // as the first argument of the function. If the function > // raises an exception, no data is added to the histogram. > // > // Since all queries are executed in order by the database backend, the > // callbacks can also use the result of previous queries stored in the > - // probeValues object. > + // probeValues object. This needs to be kept in mind while making this async. I assume this is just a temporary note
Attachment #8867521 - Flags: review?(mak77)
Comment on attachment 8867521 [details] Bug 1036440 - Refactor and use async/await and withConnectionWrapper/promiseDBConnection in PlacesDBUtils, https://reviewboard.mozilla.org/r/139070/#review144058 > I assume this is just a temporary note Sorry, yes, this will be removed. The issue I have here is that should I use `promiseDBConnection` instead of `withConnectionWrapper` (since both SELECT/PRAGMA only read from the database so we can do concurrent reads)? The problem arises since one the probes uses a value from a previous probe, so I would need to wait till those are done.
the problem with using promiseDBConnection happens only if you make a write to a table and then expect to read a value from that same table, that value could be stale. So it depends on the situation, reading the same data you wrote may not work, otherwise it will be fine.
Comment on attachment 8867521 [details] Bug 1036440 - Refactor and use async/await and withConnectionWrapper/promiseDBConnection in PlacesDBUtils, https://reviewboard.mozilla.org/r/139070/#review144058 > So, I'd like to clarify one thing, the code here is horrible, I wrote this when we didn't have Task, async and promises. > Due to that, I'm happy if you want to also refactor some of it. For example, this _executeTasks method takes a list of tags, executs them, and for async tasks it waits and then calls back itself. That's horrible. > With promises and async we could probably make this just run a simple loop and wait for every promise, without having to pop a task and call back to ourselves. Should I remove the entire Tasks object? Because of all the methods not prefixed by `_`, only `shutdown`, `maintenenceOnIdle` and `checkAndFixDatabase` seem to be used by consumers (none of them has the `aTasks` argument). Are there likely to be add ons using this part of the api or stuff that would be broken because of this?
Attachment #8869749 - Flags: review?(mak77)
Comment on attachment 8867521 [details] Bug 1036440 - Refactor and use async/await and withConnectionWrapper/promiseDBConnection in PlacesDBUtils, https://reviewboard.mozilla.org/r/139070/#review144058 > I'm wondering whether we want to use Promise.all here or not. > The problem with Promise.all is that it will stop at the first rejection. The previous code continued instead. > Maybe you should just await and try/catch in the for loop to stay consistent on how things were before. Fixed in the first changeset by an empty .catch() on each of the promises in allStatementsPromises so that the promise will always resolve.
(In reply to Milind L (:milindl) from comment #16) > Comment on attachment 8869749 [details] > Bug 1036440-Refactor PlacesDBUtils to use await/async instead of Tasks > object [will squash if reasonable], > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/141316/diff/1-2/ This is a proposed patch for refactoring the file completely (removing the Tasks object) and switching completely to async/await and loops. The _executeTasks method is no longer required and when I need to run multiple functions I use the modified runTasks method. Some things to be noted: * a `logs` array is passed around in the functions so that we can get a log for the callback at the end. This could be replaced by a PlacesDBUtils._logs array of strings if it's needed. * This patch has a lot of changes, so I realize I may be going in the wrong direction. Therefore it is on a separate changeset so I can quickly discard it (if I amend the first changeset, it may be messy to go back) * Again, all the tests using PlacesDBUtils are PASSing locally :)
Comment on attachment 8867521 [details] Bug 1036440 - Refactor and use async/await and withConnectionWrapper/promiseDBConnection in PlacesDBUtils, https://reviewboard.mozilla.org/r/139070/#review145114 ::: toolkit/components/places/PlacesDBUtils.jsm:239 (Diff revision 6) > + "PlacesDBUtils: coherence check", > + async (db) => { > + await db.executeCached(query, params).catch(() => { > + coherenceCheck = false; > + }); > + }).catch(() => { /* Ignore and continue */ }); let's at least reportError, it shouldn't hurt ::: toolkit/components/places/PlacesDBUtils.jsm:913 (Diff revision 6) > + params.type = "trigger"; > + allStatementPromises.push(async function() { > + let db = await PlacesUtils.promiseDBConnection(); > + return db.executeCached(query, params, r => { > + tasks.log(`Trigger ${r.getResultByIndex(0)}`); > + }); the indentation style here is not consistent with the previous chunk of code. ::: toolkit/components/places/PlacesDBUtils.jsm:917 (Diff revision 6) > + tasks.log(`Trigger ${r.getResultByIndex(0)}`); > + }); > + }()); > > - PlacesDBUtils._executeTasks(tasks); > + await Promise.all(allStatementPromises); > + await Promise.all(tableCountPromises); shouldn't we .catch and reportError each promise here as well?
Attachment #8867521 - Flags: review?(mak77) → review+
Hi Marco, before I fix the changes and we can run it on try, please take a look at the other patch also. I've just realized that '[will squash if reasonable]' sounds like "I will squash it once it's reasonable" but what I actually meant it "this has a large number of changes, please take a look if it's reasonable approach and I'll squash if it is, and discard if it isn't." Thanks!
Yes sure. I had to interrupt the review for other stuff, I'll be back to it soon.
Comment on attachment 8869749 [details] Bug 1036440-Refactor PlacesDBUtils to use await/async instead of Tasks object [will squash if reasonable], https://reviewboard.mozilla.org/r/141316/#review145122 ::: toolkit/components/places/PlacesDBUtils.jsm:51 (Diff revision 2) > + */ > + clearPendingTasks(logs, message = "- Unable to continue pending tasks") { > + log(logs, message); > + PlacesDBUtils._clearTaskQueue = true; > + }, > /** missing newline ::: toolkit/components/places/PlacesDBUtils.jsm:73 (Diff revision 2) > - Services.prefs.setIntPref("places.database.lastMaintenance", > + Services.prefs.setIntPref("places.database.lastMaintenance", > - parseInt(Date.now() / 1000)); > + parseInt(Date.now() / 1000)); > - if (aCallback) { > - aCallback(); > - } > - }; > + Services.telemetry.getHistogramById("PLACES_IDLE_MAINTENANCE_TIME_MS") > + .add(Date.now() - telemetryStartTime); > + // DOUBT: Should we notify the observers only when maintenanceOnIdle ends? > + // Or is it every time we finish all the tasks (end of checkAndFixDatabase also) just idle is fine ::: toolkit/components/places/PlacesDBUtils.jsm:132 (Diff revision 2) > }).catch(() => { > - tasks.log("- Unable to reindex database"); > + log(logs, "- Unable to reindex database"); > }); > - await PlacesDBUtils._executeTasks(tasks); > }); > }, So I was wondering if we could go further, the whole text-encoded system ("+", "-", ">" and so on) is an horrible js interface. We could have runTasks return a Map, each task would have a key in this Map named after the task, and as value an object like { succeeded: (bool), messages: (array) } Then for legacy consumers we could provide an util to convert this Map to the legacy textual format. Each task would either reject with an optional message, or resolve to an array of messages. Consumers of single tasks not going through runTasks will handle the promises and output by themselves. This would require changing a couple consumers in the code, but should not be anything crazy. What do you think, does this make sense?
Attachment #8869749 - Flags: review?(mak77)
It would've been really ideal to use the Option or Either types if JavaScript had them, they would be suited to this case I think. On the other hand, this is a nicer approach than what I am doing now. In any case I will squash my commit onto the first one and come up with an implementation.
Attachment #8869749 - Attachment is obsolete: true
Attachment #8867521 - Flags: review+ → review?(mak77)
If get the old logs and modify the tests a little bit (eg moving from a callback based approach to a promise based approach), I am able to get tests to pass locally. I'll update the tests and the consumers in a different patch. Will any add-ons etc break because of this patch? (In that case we should return the legacy logs by default, the Map should be opt-in, right? Or is there a way to manage this somehow) Please take a look - since this is almost a complete rewrite, there may be a large scope for improvement. I am personally finding blocks of code like ``` await PlacesDBUtils.withConnectionWrapper(string, async (db) => { db.executeCached(query, params, row => {}).then(...).catch(...) }).catch(...) ``` blocks to be a bit unwieldy, if there's a better way, please suggest that so I can make it look prettier as well :) One doubt: since async methods return a Promise anyway, when I am returning the logs at the end, should I simply `return logs;` or should I `return Promise.resolve(logs);` as I have done now? Other doubts are inside the file itself. Thanks!
Attachment #8867521 - Flags: review?(mak77)
(In reply to Milind L (:milindl) from comment #24) > I am personally finding blocks of code like > > ``` > await PlacesDBUtils.withConnectionWrapper(string, async (db) => { > db.executeCached(query, params, row => {}).then(...).catch(...) > }).catch(...) > ``` > > blocks to be a bit unwieldy, if there's a better way, please suggest that so > I can make it look prettier as well :) it depends case-by-case, but in general since the callback is an async function you can use try/catch and await inside it. you can also let exceptions go through and they will be catched by the external .catch. > One doubt: since async methods return a Promise anyway, when I am returning > the logs at the end, should I simply > > `return logs;` or should I `return Promise.resolve(logs);` as I have done > now? Other doubts are inside the file itself. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function "When an async function is called, it returns a Promise. When the async function returns a value, the Promise will be resolved with the returned value. When the async function throws an exception or some value, the Promise will be rejected with the thrown value." You don't need to return a promise, async function already returns a promise.
also, don't worry about add-ons, the only one with a meaningful number of users is Places Maintenance, and it's made by me, so I guess I'll have to update it during the week end :)
Comment on attachment 8867521 [details] Bug 1036440 - Refactor and use async/await and withConnectionWrapper/promiseDBConnection in PlacesDBUtils, https://reviewboard.mozilla.org/r/139070/#review145906 ::: toolkit/components/places/PlacesDBUtils.jsm:60 (Diff revision 8) > * Callback to be invoked when done. The callback will get a array > * of log messages. > - * @param [optional] aScope > + * @param [optional] scope > * Scope for the callback. > */ > - maintenanceOnIdle: function PDBU_maintenanceOnIdle(aCallback, aScope) { > + async maintenanceOnIdle(callback, scope = null) { we can probably remove aScope, consumers can use arrow functions. ::: toolkit/components/places/PlacesDBUtils.jsm:69 (Diff revision 8) > - , this._refreshUI > - ]); > - tasks._telemetryStart = Date.now(); > - tasks.callback = function() { > + this._refreshUI > + ]; > + let telemetryStartTime = Date.now(); > + let taskStatusMap = await PlacesDBUtils.runTasks(tasks); > + if (callback) { > + // DOUBT: ok to make callback async? Or should I keep it sync like the legacy version? I don't see a reason to make it async... I'll say more, now that these are async functions there's no reason to have a callback at all... let's just remove all the arguments. ::: toolkit/components/places/PlacesDBUtils.jsm:77 (Diff revision 8) > - Services.prefs.setIntPref("places.database.lastMaintenance", > + Services.prefs.setIntPref("places.database.lastMaintenance", > - parseInt(Date.now() / 1000)); > + parseInt(Date.now() / 1000)); > - if (aCallback) > - aCallback(); > - } > - tasks.scope = aScope; > + Services.telemetry.getHistogramById("PLACES_IDLE_MAINTENANCE_TIME_MS") > + .add(Date.now() - telemetryStartTime); > + Services.obs.notifyObservers(null, FINISHED_MAINTENANCE_TOPIC); > + // DOUBT: I was thinking maybe we could return tasksStatusMap, and not callback. Indeed, we should return the map and remove the arguments to the function. It will require fixing the only consumer using the callback, that is the test itself. ::: toolkit/components/places/PlacesDBUtils.jsm:90 (Diff revision 8) > * Callback to be invoked when done. The callback will get a array > * of log messages. > - * @param [optional] aScope > + * @param [optional] scope > * Scope for the callback. > */ > - checkAndFixDatabase: function PDBU_checkAndFixDatabase(aCallback, aScope) { > + async checkAndFixDatabase(callback, scope = null) { ditto for removing all the arguments. some consumers need fixes ::: toolkit/components/places/PlacesDBUtils.jsm:127 (Diff revision 8) > * > - * @param [optional] aTasks > - * Tasks object to execute. > + * @returns {Promise} resolved when reindex is complete. > + * @resolves to an array of logs for this task. > + * @rejects if we're unable to reindex the database. > */ > - reindex: function PDBU_reindex(aTasks) { > + async reindex(logs) { no need to pass logs ::: toolkit/components/places/PlacesDBUtils.jsm:129 (Diff revision 8) > - * Tasks object to execute. > + * @resolves to an array of logs for this task. > + * @rejects if we're unable to reindex the database. > */ > - reindex: function PDBU_reindex(aTasks) { > - let tasks = new Tasks(aTasks); > - tasks.log("> Reindex"); > + async reindex(logs) { > + let _logs = []; > + _logs.push("> Reindex"); we should not need to push a title. We can rebuild the titles from the Map keys. ::: toolkit/components/places/PlacesDBUtils.jsm:136 (Diff revision 8) > - let stmt = DBConn.createAsyncStatement("REINDEX"); > - stmt.executeAsync({ > - handleError: PlacesDBUtils._handleError, > - handleResult() {}, > - > - handleCompletion(aReason) { > + return PlacesUtils.withConnectionWrapper( > + "PlacesDBUtils: Reindex the database", > + async (db) => { > + let query = "REINDEX"; > + await db.executeCached(query).then(() => { > + _logs.push("+ The database has been re indexed"); let's about the + and -, we can infer those from the "succeeded" property in the Map for each task ::: toolkit/components/places/PlacesDBUtils.jsm:139 (Diff revision 8) > - > - PlacesDBUtils._executeTasks(tasks); > - } > - }); > + }); > - stmt.finalize(); > + }).then(() => _logs) > + .catch(() => Promise.reject("- Unable to reindex database.")); You could probably try { let logs = []; await PlacesUtils.withConnectionWrapper( "PlacesDBUtils: Reindex the database", async (db) => { await db.executeCached("REINDEX"); logs.push("The database has been reindexed"); } ); return logs; } catch (ex) { throw new Error("Unable to reindex the database."); } so, either resolve to an array of messages, or throw an Error. ::: toolkit/components/places/PlacesDBUtils.jsm:162 (Diff revision 8) > + * @resolves to an array of logs for this task. > + * @rejects if we're unable to fix corruption or unable to check status. > */ > - checkIntegrity: function PDBU_checkIntegrity(aTasks, aSkipReindex) { > - let tasks = new Tasks(aTasks); > - tasks.log("> Integrity check"); > + async checkIntegrity(skipReindex) { > + let _logs = []; > + let returnPromise = Promise.resolve(_logs); you don't need all of this complication with Promise.resolve or Promise.reject ::: toolkit/components/places/PlacesDBUtils.jsm:174 (Diff revision 8) > - > - _corrupt: false, > - handleResult(aResultSet) { > - let row = aResultSet.getNextRow(); > - this._corrupt = row.getResultByIndex(0) != "ok"; > - }, > + null, > + row => { > + if (row.getResultByIndex(0) !== "ok") { > + throw StopIteration; > + } > + }).then(() => { I think you could let rows = await db.executeCached( "PRAGMA integrity_check", null, row => { // We only need one row. throw StopIteration; }); if (rows[0].getResultByIndex(0) == "ok") { ... } else { // try to fix it } I don't think that StopIteration is forwarded, otherwise it would be a bug in Sqlite.jsm imo. ::: toolkit/components/places/PlacesDBUtils.jsm:190 (Diff revision 8) > - } > - > - PlacesDBUtils._executeTasks(tasks); > - } > + } > + // Try to reindex, this often fixes simple indices corruption. > + // DOUBT: Do we need to log these this reindex as well (it's not in the original list of tasks)? yes, just append the messaged, it doesn't matter much. It's likely we'll reduce the exposed APIs to checkAndFixDatabase(), maintenanceOnIdle(), telemetry() and shutdown(). All the rest will become internal details... ::: toolkit/components/places/PlacesDBUtils.jsm:213 (Diff revision 8) > */ > - checkCoherence: function PDBU_checkCoherence(aTasks) { > - let tasks = new Tasks(aTasks); > - tasks.log("> Coherence check"); > - > - let stmts = PlacesDBUtils._getBoundCoherenceStatements(); > + async checkCoherence() { > + let _logs = []; > + let returnPromise = Promise.resolve(_logs); > + > + _logs.push("> Coherence check"); ditto ::: toolkit/components/places/PlacesDBUtils.jsm:774 (Diff revision 8) > - * Tasks object to execute. > + * @resolves to an array of logs for this task. > + * @rejects if we are unable to vacuum database. > */ > - vacuum: function PDBU_vacuum(aTasks) { > - let tasks = new Tasks(aTasks); > - tasks.log("> Vacuum"); > + async vacuum() { > + let _logs = []; > + _logs.push("> Vacuum"); ditto ::: toolkit/components/places/PlacesDBUtils.jsm:805 (Diff revision 8) > - * @param [optional] aTasks > - * Tasks object to execute. > + * @returns {Promise} resolves when the database in cleaned up. > + * @resolves to an array of logs for this task. > */ > - expire: function PDBU_expire(aTasks) { > - let tasks = new Tasks(aTasks); > - tasks.log("> Orphans expiration"); > + async expire() { > + let _logs = []; > + _logs.push("> Orphans expiration"); ditto ::: toolkit/components/places/PlacesDBUtils.jsm:833 (Diff revision 8) > - * Tasks object to execute. > + * @resolves to an array of logs for this task. > + * @rejects if we are unable to collect stats for some reason. > */ > - stats: function PDBU_stats(aTasks) { > - let tasks = new Tasks(aTasks); > - tasks.log("> Statistics"); > + async stats() { > + let _logs = []; > + _logs.push("> Statistics"); ditto ::: toolkit/components/places/PlacesDBUtils.jsm:853 (Diff revision 8) > - stmt.finalize(); > + "PlacesDBUtils: pragma for stats", > + async (db) => { > + await db.executeCached( > + pragmaQuery, > + null, > + row => { just use let row = await db.executeCached(...)[0]; for (let i = 0; i != pragmas.length; i++) { ::: toolkit/components/places/PlacesDBUtils.jsm:871 (Diff revision 8) > - tasks.log("History can store a maximum of " + limitURIs + " unique pages"); > + _logs.push("History can store a maximum of " + limitURIs + " unique pages"); > } catch (ex) {} > > - let stmt = DBConn.createStatement( > - "SELECT name FROM sqlite_master WHERE type = :type"); > - stmt.params.type = "table"; > + let query = "SELECT name FROM sqlite_master WHERE type = :type"; > + let params = {}; > + let allStatementPromises = []; I don't think we need all this parallelism here, so likely code can be simplified. We just care to run queries up the where possible, if anything fails it means the db is corrupt so we can bail out. ::: toolkit/components/places/PlacesDBUtils.jsm:882 (Diff revision 8) > - tasks.log("Table " + tableName + " has " + countStmt.getInt32(0) + " records"); > - countStmt.finalize(); > - } > - stmt.reset(); > - > - stmt.params.type = "index"; > + _logs.push(`Table ${tableName} has ${rows[0].getResultByIndex(0)} records`); > + }); > + tableCountPromises.push(rowsPromise); > + }; > + // All of these operations should be done in parallel, hence we make promises > + // and collect them inside an array before awaiting them. I don't think it matters much to run these in parallel, also because it won't really happen (sqlite serializes everything). I'd simplify this to: let rows = await db.executeCached... Note: most of the queries in PlacesDBUtils are unlikely to run often, so we should probably just use execute() everywhere instead of executeCached. ::: toolkit/components/places/PlacesDBUtils.jsm:1038 (Diff revision 8) > }; > > for (let i = 0; i < probes.length; i++) { > let probe = probes[i]; > > - let promiseDone = new Promise((resolve, reject) => { > + let promiseDone = new Promise(async (resolve, reject) => { I don't think this works? Or at least it's stretching a little bit the syntax. let's avoid the await later instead... ::: toolkit/components/places/PlacesDBUtils.jsm:1051 (Diff revision 8) > - if (probe.query.indexOf(":" + param) > 0) { > - stmt.params[param] = params[param]; > + if (probe.query.includes(`:${p}`)) { > + filteredParams[p] = params[p]; > } > } > - > - try { > + let db = await PlacesUtils.promiseDBConnection(); > + await db.executeCached( just chain promises here PlacesUtils.promiseDBConnection() .then(db => db.executeCached(...)) .then(rows => resolve...) .catch(ex => reject(...)) ::: toolkit/components/places/PlacesDBUtils.jsm:1087 (Diff revision 8) > - tasks.callback = aCallback; > - PlacesDBUtils._executeTasks(tasks); > + let tasksMap = new Map(); > + // First, call all tasks one-by-one. > + for (let task of tasks) { > + if (PlacesDBUtils._isShuttingDown) { > + // DOUBT: where exactly do I move this to? > + // _logs.push("- We are shutting down. Will not schedule the tasks."); just mark all tasks from this point on as failed and set this as their message?
Attachment #8867521 - Flags: review?(mak77)
Blocks: 1337409
Comment on attachment 8867521 [details] Bug 1036440 - Refactor and use async/await and withConnectionWrapper/promiseDBConnection in PlacesDBUtils, https://reviewboard.mozilla.org/r/139070/#review146344 we're close here, I'd not add more requirements since we risk to make this an infinite rewrite, we can always do further cleanups in new bugs. Thank you for coping with my many comments! ::: toolkit/components/places/PlacesDBUtils.jsm:120 (Diff revision 9) > * > - * @param [optional] aTasks > - * Tasks object to execute. > + * @returns {Promise} resolved when reindex is complete. > + * @resolves to an array of logs for this task. > + * @rejects if we're unable to reindex the database. > */ > - reindex: function PDBU_reindex(aTasks) { > + async reindex(logs) { the logs argument should not be there. ::: toolkit/components/places/PlacesDBUtils.jsm:122 (Diff revision 9) > - * Tasks object to execute. > + * @resolves to an array of logs for this task. > + * @rejects if we're unable to reindex the database. > */ > - reindex: function PDBU_reindex(aTasks) { > - let tasks = new Tasks(aTasks); > - tasks.log("> Reindex"); > + async reindex(logs) { > + try { > + let _logs = []; is there a reason these _logs var are prefixed by _? that's not out common coding style, we usually use _ just to indicate methods or properties considered "private" in objects. Please rename "_logs" to "logs" everywhere. ::: toolkit/components/places/PlacesDBUtils.jsm:172 (Diff revision 9) > - if (aSkipReindex) { > - tasks.log("- Unable to fix corruption, database will be replaced on next startup"); > - Services.prefs.setBoolPref("places.database.replaceOnStartup", true); > - tasks.clear(); > } else { > - // Try to reindex, this often fixed simple indices corruption. > + row = r; doesn't this work? row = r; throw StopIteration; ::: toolkit/components/places/PlacesDBUtils.jsm:196 (Diff revision 9) > - }); > + }); > - stmt.finalize(); > + } catch (ex) { > + if (ex.message.indexOf("Unable to fix corruption") !== 0) { > + // There was some other error, so throw. > + PlacesDBUtils.clearPendingTasks(); > + throw new Error("Unable to check database status"); s/status/integrity/ ::: toolkit/components/places/PlacesDBUtils.jsm:205 (Diff revision 9) > }, > > /** > * Checks data coherence and tries to fix most common errors. > * > - * @param [optional] aTasks > + * @returns {Promise} resolves when coherence is checked. afaik "@return" is correct, "@returns" is not please fix this in the other points too ::: toolkit/components/places/PlacesDBUtils.jsm:215 (Diff revision 9) > - let tasks = new Tasks(aTasks); > - tasks.log("> Coherence check"); > - > - let stmts = PlacesDBUtils._getBoundCoherenceStatements(); > - DBConn.executeAsync(stmts, stmts.length, { > - handleError: PlacesDBUtils._handleError, > + let _logs = []; > + > + let stmts = await PlacesDBUtils._getBoundCoherenceStatements(); > + let allStatementsPromises = []; > + let coherenceCheck = true; > + for (let {query, params} of stmts) { there is an interesting difference here, DBConn.executeAsync was running all the statements in a transaction, a simple loop here won't. You need to use db.executeTransaction around the for loop. ::: toolkit/components/places/PlacesDBUtils.jsm:1062 (Diff revision 9) > - } > + } > -}; > > -/** > - * LIFO tasks stack. > - * > + if (PlacesDBUtils._clearTaskQueue) { > + tasksError = "The task queue was cleared by an error in another task."; > + break; I suppose that, instead of breaking, here and in the previous condition, you may continue. And instead of setting tasksError you could directly tasksMap.set(task.name, { succeeded: false, logs: ["your error"] }); then you'd not need to loop twice and keep a separate index var (probably at that point you could use for...of).
Attachment #8867521 - Flags: review?(mak77)
Comment on attachment 8867521 [details] Bug 1036440 - Refactor and use async/await and withConnectionWrapper/promiseDBConnection in PlacesDBUtils, https://reviewboard.mozilla.org/r/139070/#review146356 ::: toolkit/components/places/PlacesDBUtils.jsm:21 (Diff revision 9) > > this.EXPORTED_SYMBOLS = [ "PlacesDBUtils" ]; > > // Constants > > const FINISHED_MAINTENANCE_TOPIC = "places-maintenance-finished"; can we remove this? If all the consumers can be converted to use a promise, probably we could.
Comment on attachment 8870690 [details] Bug 1036440 - Fix tests and consumers for rewritten `PlacesDBUtils`, https://reviewboard.mozilla.org/r/142164/#review146358 ::: toolkit/components/places/tests/unit/test_preventive_maintenance_checkAndFixDatabase.js:23 (Diff revision 2) > - let positives = []; > - let negatives = []; > - let infos = []; > - > - aLog.forEach(function(aMsg) { > - print(aMsg); > + let failedTasks = []; > + tasksStatusMap.forEach(val => { > + if (val) { > + successfulTasks.push(val); > + } else { > + failedTasks.push(val); shouldn't we check succeeded as well? ::: toolkit/components/places/tests/unit/test_preventive_maintenance_runTasks.js:21 (Diff revision 2) > - let positives = []; > - let negatives = []; > - let infos = []; > - > - aLog.forEach(function(aMsg) { > - print(aMsg); > + let failedTasks = []; > + tasksStatusMap.forEach(val => { > + if (val) { > + successfulTasks.push(val); > + } else { > + failedTasks.push(val); ditto
Attachment #8870690 - Flags: review?(mak77)
Comment on attachment 8867521 [details] Bug 1036440 - Refactor and use async/await and withConnectionWrapper/promiseDBConnection in PlacesDBUtils, https://reviewboard.mozilla.org/r/139070/#review146344 > there is an interesting difference here, DBConn.executeAsync was running all the statements in a transaction, a simple loop here won't. > You need to use db.executeTransaction around the for loop. Thanks for noticing this, I got to learn about transactions and how they're different from the usual statements. The current diff shoudl address comments, and consists mostly of trivial fixes, this is the only one I'm a bit unsure about.
Latest diff fixes some linting issues in the test files. Fixed to avoid try failure at a later stage.
(In reply to Milind L (:milindl) from comment #37) > Thanks for noticing this, I got to learn about transactions and how they're > different from the usual statements. In an ACID database a transaction is an atomic operation, if it succeeds you're sure your data is written and persisted, if it fails no changes to the underlying data is made. To ensure atomicity a transaction has an overhead cost. A write statement (INSERT/UPDATE) in Sqlite has always an intrinsic transaction in it. If you run 10 write statements you will pay 10 times the overhead cost. By wrapping those 10 statements in an external transaction you pay the overhead cost just once (but clearly failing a single statement will rollback the entire transaction).
Comment on attachment 8867521 [details] Bug 1036440 - Refactor and use async/await and withConnectionWrapper/promiseDBConnection in PlacesDBUtils, https://reviewboard.mozilla.org/r/139070/#review146532 LGTM, I'll start a Try server build. ::: toolkit/components/places/PlacesDBUtils.jsm:886 (Diff revision 10) > * Collects telemetry data and reports it to Telemetry. > * > - * @param [optional] aTasks > - * Tasks object to execute. > */ > - telemetry: function PDBU_telemetry(aTasks) { > + async telemetry(logs) { still one argument to remove here
Attachment #8867521 - Flags: review?(mak77) → review+
Comment on attachment 8870690 [details] Bug 1036440 - Fix tests and consumers for rewritten `PlacesDBUtils`, https://reviewboard.mozilla.org/r/142164/#review146536
Attachment #8870690 - Flags: review?(mak77) → review+
there's one eslint failure: TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/content/aboutSupport.js:1039:65 | Strings must use doublequote. (quotes) [log…]
Summary: Rewriting PlacesDBUtils using Tasks.jsm and promiseDBConnection → Rewriting PlacesDBUtils using async/await and promiseDBConnection
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 4cd484a27abd -d 6f3faf69d7e5: rebasing 398413:4cd484a27abd "Bug 1036440 - Refactor and use async/await and withConnectionWrapper/promiseDBConnection in PlacesDBUtils, r=mak" merging toolkit/components/places/PlacesDBUtils.jsm warning: conflicts while merging toolkit/components/places/PlacesDBUtils.jsm! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
ah there's a rebase needed for this: https://hg.mozilla.org/mozilla-central/diff/4ca6d53664ac/toolkit/components/places/PlacesDBUtils.jsm - WHERE name BETWEEN 'weave/' AND 'weave0' + WHERE name = 'downloads/destinationFileName' OR + name BETWEEN 'weave/' AND 'weave0' Milind, could you please rebase the patch?
I ran `hg rebase -s (rev) -d central` and manually applied this (since I could not get any diffing tools to work, and for some reason I wasn't getting the conflict markers in my file). - WHERE name BETWEEN 'weave/' AND 'weave0' + WHERE name = 'downloads/destinationFileName' OR + name BETWEEN 'weave/' AND 'weave0' If this was the only change we should be fine, but tell me if anything else is needed. Thank you :)
I just figured out the way to check this is to check the logs for all files I've changed and see if they have changed since I submitted my first diff (when I hg pulled). I am doing that right now, I'll inform you if I get something else I need to change.
Seems like it is OK, most of the latest changes in the test files are from the Tasks.jsm to async conversion patch, which had already pulled before starting this.
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/8bc2a9f1ecd9 Refactor and use async/await and withConnectionWrapper/promiseDBConnection in PlacesDBUtils, r=mak https://hg.mozilla.org/integration/autoland/rev/534b3577bb9a Fix tests and consumers for rewritten `PlacesDBUtils`, r=mak
Assignee: nobody → i.milind.luthra
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: