1068620 - Provide a Sqlite.wrapStorageConnection method

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Marco Bonardo [:mak]]

I need this for bug 1068009.

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1

This is simple enough I think.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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.
});

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v2

Right, I'm too much used to our "old" promises.

Comment 5

[Marco Bonardo [:mak]]

ehr, I fixed the indentation problem locally.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

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`.

Comment 7

[Marco Bonardo [:mak]]

(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.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

> 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.

Comment 9

[Marco Bonardo [:mak]]

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.

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v3

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.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 12

[Marco Bonardo [:mak]]

with fixed comments:
https://hg.mozilla.org/integration/fx-team/rev/fd762b4aa27e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fd762b4aa27e