1727261 - Sqlite.executeTransaction keeps the transaction open indefinitely if func() doesn't resolve

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, task, P2)

Description

[Marco Bonardo [:mak]]

There's no protection in executeTransaction() against a func() that never resolves, the transaction would just stay open for the whole session.
We could .race() with a timeout, the ideal outcome at that point would be a ROLLBACK (because the transaction didn't complete), the problem is that if the timeout is long that rollback may affect many other changes that happened in the same timeframe. On the other side a COMMIT may commit a partial transaction and that's not ideal for data coherency.
The rollback solution could be acceptable if the timeout is not too long, but we'd need to land a telemetry probe to measure how many times we cross it and keep an eye over it. We may also want to assign a label to each executeTransaction call and report to the console when some transaction is timed-out, for debug purposes (ideally we'd telemetry the timing out labels somehow).

Comment 1

[Marco Bonardo [:mak]]

Attachment: Bug 1727261 - Rename Sqlite.jsm::_hasInProgressTransaction to _initiatedTransaction. r=asuth

initiatedTransaction seems less confusing about the fact Sqlite.jsm initiated this
transaction, rather than something else (that may happen with wrapped connections).

Comment 2

[Marco Bonardo [:mak]]

Attachment: Bug 1727261 - Move Sqlite.jsm logging off Log.jsm. r=asuth,standard8

Remove the dependency on Log.jsm and follow the most common path of providing
a simple boolean pref to increase logging. Warn and Error are logged to the
console by default, while Debug must be enabled.

Depends on D124184

Comment 3

[Marco Bonardo [:mak]]

Attachment: Bug 1727261 - Replace OS.* usage with IOUtils in test_sqlite.js. r=standard8

Depends on D124185

Comment 4

[Marco Bonardo [:mak]]

Attachment: Bug 1727261 - Timeout Sqlite transaction async functions to properly roll back and interrupt the transaction. r=asuth

If the passed-in async function never resolves the transaction could stay alive forever, this timeouts after
TRANSACTIONS_QUEUE_TIMEOUT_MS (5 minutes as of now) and roll backs the transaction.
A telemetry keyed scalar is added to track callers causing these timeouts and optimize them in the future.

Depends on D124186

Comment 5

[Marco Bonardo [:mak]]

Attachment: data-review.txt

Please forward if you're too busy for this.

Comment 6

[Chris H-C :chutten]

Comment on attachment 9238900 [details]
data-review.txt

DATA COLLECTION REVIEW RESPONSE:

Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes.

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, Marco Bonardo is responsible.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

Is the data collection request for default-on or default-off?

Default on for all channels.

Does the instrumentation include the addition of any new identifiers?

No.

Is the data collection covered by the existing Firefox privacy notice?

Yes.

Does the data collection use a third-party collection tool?

No.

---

Result: datareview+

Comment 7

[Marco Bonardo [:mak]]

Attachment: Bug 1727261 - Allow to control Sqlite.jsm logging through a pref. r=asuth!,standard8!

Add a pref to manage Sqlite.jsm logging, default to reporting Errors.
Change some of the logging to more coherently report warnings, errors and debug info.
Reuse the same logger for all the objects, just change the prefix.

Depends on D124184

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e35a78cb5399
Rename Sqlite.jsm::_hasInProgressTransaction to _initiatedTransaction. r=asuth
https://hg.mozilla.org/integration/autoland/rev/4b46bd28680f
Allow to control Sqlite.jsm logging through a pref. r=asuth,Standard8
https://hg.mozilla.org/integration/autoland/rev/204be801f197
Replace OS.* usage with IOUtils in test_sqlite.js. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/c7a7c20d73c0
Timeout Sqlite transaction async functions to properly roll back and interrupt the transaction. r=asuth

Comment 9

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/e35a78cb5399
https://hg.mozilla.org/mozilla-central/rev/4b46bd28680f
https://hg.mozilla.org/mozilla-central/rev/204be801f197
https://hg.mozilla.org/mozilla-central/rev/c7a7c20d73c0

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: Bug 1727261 - make toolkit/modules/tests/xpcshell/test_sqlite.js pass for Thunderbird even if it's not collecting the Telemetry. r=Standard8

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/423c9445cbf6
make toolkit/modules/tests/xpcshell/test_sqlite.js pass for Thunderbird even if it's not collecting the Telemetry. r=mak DONTBUILD

Comment 12

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/423c9445cbf6