Closed Bug 1442353 Opened 5 years ago Closed 5 years ago

Reuse timeoutPromise in Sqlite.jsm

Categories

(Toolkit :: General, enhancement)

Product:
Toolkit
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

References

Details

Attachments

(1 file)

Bug 1442353 - Reuse timeoutPromise in Sqlite.jsm
59 bytes, text/x-review-board-request
florian
: review+
Details 
Follow up from bug 1441712. Ideally, we wouldn't do one setTimeout per transaction in Sqlite.jsm.

Bug 1441712 comment 4 suggests one approach to avoid this (which I'm about to attach a patch implementing), but it occurs to me this approach requires lowering the effective TRANSACTIONS_QUEUE_TIMEOUT_MS to half of what it is now, and after asking around, even its current value may be too low in cases like PlacesUtils.bookmarks.insertTree (which kitcambridge reports having seen taking more than 4min to complete).
(In reply to Thom Chiovoloni [:tcsc] from comment #0)
> it occurs to me this approach requires
> lowering the effective TRANSACTIONS_QUEUE_TIMEOUT_MS to half of what it is
> now, and after asking around, even its current value may be too low in cases
> like PlacesUtils.bookmarks.insertTree (which kitcambridge reports having
> seen taking more than 4min to complete).

So do you want to increase this value? Or re-use the timer only if at least 80% (or some other proportion) of the time remains?
Mak: what do you think? We'd effectively be lowering TRANSACTIONS_QUEUE_TIMEOUT_MS, but it's also possible it's too low currently.
Flags: needinfo?(mak77)
TL;DR: imo we need a timed promise, or a cheap way to build one.

lowering TRANSACTIONS_QUEUE_TIMEOUT_MS doesn't sound good, we had to increase it not much time ago. Each promise we enqueue here should have TRANSACTIONS_QUEUE_TIMEOUT_MS time to complete _from when it starts running_ not from when it's enqueued.
Additionally, this suggested method looks less understandable than the existing one, I don't like it much, and Date.now() is not reliable.

The problem we are trying to solve is a queue of promises, it was suggested in the past to create a module to handle that, but it never happened, it would be extremely useful and we could relegate handling of these issues inside it.
In the queue of promises world, if one of the promises is never resolved, or if you create a cycle (by mistake promise2 is started from promise1 and then enqueued to promise1) your queue is stuck and your product just breaks forever.
There is more discussion in bug 1091446 and bug 1102186. Paolo had some ideas regarding task queue.

The cycle problem can probably be detected with the appropriate tooling in promises themselves. The never resolved promised problem must be handled with timeouts, or the promise should track its running time and forcibly rejected when it's evaluated and it's running for too long.
Flags: needinfo?(mak77)
Comment on attachment 8955259 [details]
Bug 1442353 - Reuse timeoutPromise in Sqlite.jsm

https://reviewboard.mozilla.org/r/224404/#review230676

::: toolkit/modules/Sqlite.jsm:853
(Diff revision 1)
>      this._idleShrinkTimer.initWithCallback(this.shrinkMemory.bind(this),
>                                             this._idleShrinkMS,
>                                             this._idleShrinkTimer.TYPE_ONE_SHOT);
> +  },
> +
> +  _getTimeoutPromise() {

Marco had a concern about code clarity. I think we can improve clarity by adding a comment above "_getTimeoutPromise()" that specifies what this promise is expected to do. Eg. "returns a promise that will resolve after a time comprised between 80% of TRANSACTIONS_QUEUE_TIMEOUT_MS and TRANSACTIONS_QUEUE_TIMEOUT_MS. Use this promise instead of creating several individual timers to reduce the overhead due to timers."

::: toolkit/modules/Sqlite.jsm:854
(Diff revision 1)
>                                             this._idleShrinkMS,
>                                             this._idleShrinkTimer.TYPE_ONE_SHOT);
> +  },
> +
> +  _getTimeoutPromise() {
> +    if (this._timeoutPromise && Date.now() <= this._timeoutPromiseExpires) {

This should use Cu.now() instead of Date.now() on both calls.

::: toolkit/modules/Sqlite.jsm:858
(Diff revision 1)
> +  _getTimeoutPromise() {
> +    if (this._timeoutPromise && Date.now() <= this._timeoutPromiseExpires) {
> +      return this._timeoutPromise;
> +    }
> +    this._timeoutPromise = new Promise((resolve, reject) => {
> +      setTimeout(() => reject(new Error("Transaction timeout, most likely caused by unresolved pending work.")),

When the setTimeout callback is called, we should reset this._timeoutPromise to null.

::: toolkit/modules/Sqlite.jsm:861
(Diff revision 1)
> +    }
> +    this._timeoutPromise = new Promise((resolve, reject) => {
> +      setTimeout(() => reject(new Error("Transaction timeout, most likely caused by unresolved pending work.")),
> +                 TRANSACTIONS_QUEUE_TIMEOUT_MS);
> +    });
> +    this._timeoutPromiseExpires = Date.now() + TRANSACTIONS_QUEUE_TIMEOUT_MS / 2;

After discussing with Marco, he's not comfortable reducing the timeout under 4 minutes without first collecting telemetry data about how long transactions take in the wild.

I propose we bump the timeout to 5 minutes and reset the timer if less than 4 minutes remain, ie replace "/ 2" with "* 0.2".
Attachment #8955259 - Flags: review?(florian) → review-
(In reply to Marco Bonardo [::mak] from comment #4)
> The problem we are trying to solve is a queue of promises, it was suggested
> in the past to create a module to handle that, but it never happened, it
> would be extremely useful and we could relegate handling of these issues
> inside it.
> In the queue of promises world, if one of the promises is never resolved, or
> if you create a cycle (by mistake promise2 is started from promise1 and then
> enqueued to promise1) your queue is stuck and your product just breaks
> forever.
> There is more discussion in bug 1091446 and bug 1102186. Paolo had some
> ideas regarding task queue.
> 

Yeah, we have one we use in a few places in sync [0], but it doesn't detect cycles and I don't like it for that reason. It's ridiculously easy to mess this up by accident, since you need where all the promises you await come from, which requires non-local reasoning, and messing it up ends in deadlocks :(.

I remember asking around when this was going to land if there was a good way to detect the cycles, and I was disappointed that there wasn't.

[0]: https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/services/common/async.js#243
Comment on attachment 8955259 [details]
Bug 1442353 - Reuse timeoutPromise in Sqlite.jsm

https://reviewboard.mozilla.org/r/224404/#review230676

> When the setTimeout callback is called, we should reset this._timeoutPromise to null.

Shouldn't we only do this if the `this._timeoutPromise` has not changed? E.g. consider this sequence

1. We set `this._timeoutPromise` to promise1. 
2. 4.5min later, set `this._timeoutPromise` to promise2.
3. 0.5min passes, promise1 resolves, and clears `this._timeoutPromise`, which had been promise2. 
4. less than 0.5min passes, another transaction begins, but we are unable to reuse promise2 since it was cleared.
(In reply to Thom Chiovoloni [:tcsc] from comment #7)

Indeed, good catch!
Comment on attachment 8955259 [details]
Bug 1442353 - Reuse timeoutPromise in Sqlite.jsm

https://reviewboard.mozilla.org/r/224404/#review230750

Looks good to me, thanks!

::: toolkit/modules/Sqlite.jsm:266
(Diff revision 2)
>        statementCounter: this._statementCounter,
>      })
>    );
> +
> +  // We avoid creating a timer every transaction that exists solely as a safety
> +  // check (e.g. one that never should fire) by reusing it if its sufficiently

its -> it's
Attachment #8955259 - Flags: review?(florian) → review+
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a2f94b5f23a0
Reuse timeoutPromise in Sqlite.jsm r=florian
https://hg.mozilla.org/mozilla-central/rev/a2f94b5f23a0
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.