Closed
Bug 1345922
Opened 7 years ago
Closed 7 years ago
Use nsIUrlClassifierDBService.beginUpdate properly in UrlClassifierTestUtils.jsm
Categories
(Toolkit :: Safe Browsing, enhancement)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: hchang, Assigned: hchang)
References
Details
Attachments
(1 file)
http://searchfox.org/mozilla-central/rev/d4adaabd6d2f88be0d0b151e022c06f6554909da/toolkit/components/url-classifier/tests/UrlClassifierTestUtils.jsm#89 We should consider the case where beginUpdate fails due to the ongoing update.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → hchang
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8845502 -
Flags: review?(francois)
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8845502 [details] Bug 1345922 - Avoid concurrent update and take the failed beginUpdate into account. https://reviewboard.mozilla.org/r/118650/#review121676 ::: toolkit/components/url-classifier/tests/UrlClassifierTestUtils.jsm:60 (Diff revision 2) > + } > + return this.useTestDatabase(tables[tableIndex]) > + .then(() => { > + tableIndex++; > + return doOneUpdate(); > + }, aErrMsg => { The indentation is off here and makes this hard to read. This line is still within `.then()` (line 57) and is the second parameter for that function. ::: toolkit/components/url-classifier/tests/UrlClassifierTestUtils.jsm:63 (Diff revision 2) > + tableIndex++; > + return doOneUpdate(); > + }, aErrMsg => { > + dump("Rejected: " + aErrMsg + ". Retry later.\n"); > + return new Promise(resolve => { > + timer.initWithCallback(resolve, 100, Ci.nsITimer.TYPE_ONE_SHOT); Is 100ms enough of a wait? Should this be 500ms or 1000ms to make sure the current update finishes? ::: toolkit/components/url-classifier/tests/UrlClassifierTestUtils.jsm:65 (Diff revision 2) > + }, aErrMsg => { > + dump("Rejected: " + aErrMsg + ". Retry later.\n"); > + return new Promise(resolve => { > + timer.initWithCallback(resolve, 100, Ci.nsITimer.TYPE_ONE_SHOT); > + }) > + .then(doOneUpdate); This should be indented at the same level as the `.then()` from line 57.
Attachment #8845502 -
Flags: review?(francois) → review+
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8845502 [details] Bug 1345922 - Avoid concurrent update and take the failed beginUpdate into account. https://reviewboard.mozilla.org/r/118650/#review121676 > The indentation is off here and makes this hard to read. > > This line is still within `.then()` (line 57) and is the second parameter for that function. I just followed the MDN style https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then ``` p1.then(function(value) { console.log(value); // Success! }, function(reason) { console.log(reason); // Error! }); ``` except for putting ".then()" in the new line, which is also common in the codebase http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/layout/style/test/animation_utils.js#259 > Is 100ms enough of a wait? Should this be 500ms or 1000ms to make sure the current update finishes? If 100ms is not enough, the promise (line 65) will be rejected and a new timer will be init again. In other words, it's a recursive promise chain. doOneUpdate() is basically a indefinite number of promise chain and will eventually get resolved when "tableIndex == tables.length". In "all other cases", a new promise will return instead, which means the top level promise is still pending. > This should be indented at the same level as the `.then()` from line 57. It is one level less then the reject handler (i.e. the same level of the promise for timer.)
Assignee | ||
Comment 5•7 years ago
|
||
Hi Francois, I didn't change anything based on your comments. I'd like to know if you agree with my explanation. I will land it right away if you buy it :) Thanks!
Flags: needinfo?(francois)
Comment 6•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8845502 [details] Bug 1345922 - Avoid concurrent update and take the failed beginUpdate into account. https://reviewboard.mozilla.org/r/118650/#review121676 > I just followed the MDN style > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then > > ``` > p1.then(function(value) { > console.log(value); // Success! > }, function(reason) { > console.log(reason); // Error! > }); > ``` > > except for putting ".then()" in the new line, which is also common in the codebase > > http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/layout/style/test/animation_utils.js#259 You're right. It turns out I read it wrong the first time and that's why I thought it was badly indented. So much for promises making everything easier to read! :) > If 100ms is not enough, the promise (line 65) will be rejected and a new timer will be init again. In other words, it's a recursive promise chain. doOneUpdate() is basically a indefinite number of promise chain and will eventually get resolved when "tableIndex == tables.length". In "all other cases", a new promise will return instead, which means the top level promise is still pending. Right, I was just thinking that we'll be re-trying 10 times per second. That seems a bit high. Maybe we could re-try every 500ms instead? It would be more likely that it would only have to wait once or twice before going through.
Comment 7•7 years ago
|
||
(In reply to Henry Chang [:henry][:hchang] from comment #5) > I didn't change anything based on your comments. I'd like to know if you > agree with my explanation. Yes, I was wrong about indentation. I'll let you pick the delay you think is best (100ms or 500ms) and land the patch. r+
Flags: needinfo?(francois)
Pushed by hchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ffb8aa89b8c1 Avoid concurrent update and take the failed beginUpdate into account. r=francois
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ffb8aa89b8c1
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•