Provide a Sqlite.wrapStorageConnection method

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({dev-doc-needed})

Trunk
mozilla35
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

We need Sqlite.jsm to be able to wrap an external connection providing its sugar-y interface.
Though it should not handle connection shutdown since that might come from anywhere.
I need this for bug 1068009.
Blocks: 1068009
Points: --- → 5
Flags: firefox-backlog+
Iteration: --- → 35.2
Flags: qe-verify?
Posted patch patch v1 (obsolete) — Splinter Review
This is simple enough I think.
Attachment #8491532 - Flags: review?(dteller)
Flags: qe-verify?
Flags: qe-verify-
Flags: in-testsuite?
Comment on attachment 8491532 [details] [diff] [review]
patch v1

Review of attachment 8491532 [details] [diff] [review]:
-----------------------------------------------------------------

Small drive-by comment:

::: toolkit/modules/Sqlite.jsm
@@ +874,5 @@
> +  connectionCounters.set(basename, number + 1);
> +  let identifier = basename + "#" + number;
> +
> +  log.info("Wrapping database: " + path + " (" + identifier + ")");
> +  let deferred = Promise.defer();

We should probably use the ES6 API here:

return new Promise(resolve => {
  // and here you wouldn't even have to call reject() yourself,
  // you could just re-throw the exception if something goes wrong.
});
Posted patch patch v2 (obsolete) — Splinter Review
Right, I'm too much used to our "old" promises.
Attachment #8491532 - Attachment is obsolete: true
Attachment #8491532 - Flags: review?(dteller)
Attachment #8491543 - Flags: review?(dteller)
ehr, I fixed the indentation problem locally.
Comment on attachment 8491543 [details] [diff] [review]
patch v2

Review of attachment 8491543 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Sqlite.jsm
@@ +199,5 @@
>      // We wait for the first statement execute to start the timer because
>      // shrinking now would not do anything.
>    }
>  
> +  this._wrappedConnection = options._wrappedConnection;

I'm not a big fan of hidden options. Could we make this a real option, say `options.autoClose`, defaulting to `true`?

Also, please 1/normalize this to a boolean; 2/ take the opportunity to document the field.
(I realize that most fields are not documented, but let's not continue in that direction).

@@ +849,5 @@
> + * Wraps an existing and open Storage connection with Sqlite.jsm API.  The
> + * wrapped connection clone has the same underlying characteristics of the
> + * original connection and is returned in form of an OpenedConnection handle.
> + *
> + * Shutdown won't be handled by us since the connection has an unknown origin.

I believe that we should make this configurable by an option.

@@ +856,5 @@
> + *        The original Storage connection to wrap.
> + *
> + * @return Promise<OpenedConnection>
> + */
> +function wrapStorageConnection(connection) {

Let's make this arg `options`, as in `closeStorageConnection`.

@@ +871,5 @@
> +  let path = connection.databaseFile.path;
> +  let basename = OS.Path.basename(path);
> +  let number = connectionCounters.get(basename) || 0;
> +  connectionCounters.set(basename, number + 1);
> +  let identifier = basename + "#" + number;

That's the third time we get this block in the code. Time to factor it.

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +918,5 @@
> + */
> +add_task(function* test_wrapStorageConnection() {
> +  let file = new FileUtils.File(OS.Path.join(OS.Constants.Path.profileDir,
> +                                             "test_wrapStorageConnection.sqlite"));
> +  let c = yield new Promise((success, failure) => {

Nit: the standard names are `resolve` and `reject`.
Attachment #8491543 - Flags: review?(dteller) → feedback+
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #6)
> Comment on attachment 8491543 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8491543 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/modules/Sqlite.jsm
> @@ +199,5 @@
> >      // We wait for the first statement execute to start the timer because
> >      // shrinking now would not do anything.
> >    }
> >  
> > +  this._wrappedConnection = options._wrappedConnection;
> 
> I'm not a big fan of hidden options. Could we make this a real option, say
> `options.autoClose`, defaulting to `true`?

I don't think it's a good idea, if an external user would set it, he could not close the connection. Which kind of option is that breaks the component you are using?
Alternatively I can add a wrappedConnections global set and register them there... then I can avoid the pref.

> > + * Shutdown won't be handled by us since the connection has an unknown origin.
> 
> I believe that we should make this configurable by an option.

I disagree with adding an option that breaks the component if someone sets it.

I will go for the wrappedConnections set so there's no option needed at all and it's all tracked internally.
> I don't think it's a good idea, if an external user would set it, he could not close the connection. Which kind of option is that breaks the component you are using?

I believe I have counter-examples, but I'm ok with the weakset.
remember that we must push usage of Sqlite.jsm vs raw api, having raw connections around in js should be deprecated and something useful only to port old code.
Posted patch patch v3Splinter Review
when I started refactoring those 3 repeated lines I ended up figuring out we had 2 different identifiers around, with different names and contents.
That doesn't make sense, so I refactored the code a little bit to have a single identifier, generated by a new helper.
Attachment #8491543 - Attachment is obsolete: true
Attachment #8492114 - Flags: review?(dteller)
Comment on attachment 8492114 [details] [diff] [review]
patch v3

Review of attachment 8492114 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, with a few changes.

::: toolkit/modules/Sqlite.jsm
@@ +196,4 @@
>    this._log.info("Opened");
>  
>    this._dbConn = connection;
> +  this._identifier = identifier;

Not: Before landing this, could you take the opportunity to document this field?

@@ +323,5 @@
>      // This guards against operations performed between the call to this
>      // function and asyncClose() finishing. See also bug 726990.
>      this._open = false;
>  
> +    let markAsClosed = () => {

Nit: Can you add a small comment mentioning explaining that we are closing at Sqlite.jsm-level but not necessarily at mozStorage-level?

@@ +331,5 @@
>        // a blocker for Barriers.connections.
>        Barriers.connections.client.removeBlocker(deferred.promise);
>        deferred.resolve();
> +    }
> +    if (wrappedConnections.has(this._identifier)) {

Don't forget to remove cleanup `wrappedConnections`.

@@ +858,5 @@
> + * Wraps an existing and open Storage connection with Sqlite.jsm API.  The
> + * wrapped connection clone has the same underlying characteristics of the
> + * original connection and is returned in form of an OpenedConnection handle.
> + *
> + * Shutdown won't be handled by us since the connection has an unknown origin.

Nit: The word "Shutdown" is a bit too overloaded for my taste. I would prefer something along the lines of "Clients are responsible for closing the underlying mozStorage connection."

@@ +894,5 @@
> +      // already handled by the opener.
> +      wrappedConnections.add(identifier);
> +      resolve(new OpenedConnection(conn, identifier));
> +    } catch (ex) {
> +      log.warn("Could not wrap database: " + CommonUtils.exceptionStr(ex));

Can you either cleanup `wrappedConnections` in case of error or call `wrappedConnections.add` only once you are certain that `new OpenedConnection` hasn't thrown?

@@ +940,5 @@
>   *
>   * @param connection
>   *        (mozIStorageConnection) Underlying SQLite connection.
> + * @param identifier
> + *        (string) The unique identifier of this database. Used for logging.

Nit: Actually, not only for logging, as it serves as key in a few tables.

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +844,5 @@
>   */
>  add_task(function* test_cloneStorageConnection() {
>    let file = new FileUtils.File(OS.Path.join(OS.Constants.Path.profileDir,
>                                               "test_cloneStorageConnection.sqlite"));
> +  let c = yield new Promise((resolve, reject) => {

Ah, thanks.
Attachment #8492114 - Flags: review?(dteller) → review+
with fixed comments:
https://hg.mozilla.org/integration/fx-team/rev/fd762b4aa27e
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla35
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/fd762b4aa27e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.