Consider auto-closing Sqlite.jsm databases

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Yoric, Assigned: brennan.brisad, Mentored)

Tracking

unspecified
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Async][lang=js])

Attachments

(4 attachments, 10 obsolete attachments)

56.11 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
12.23 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
64.70 KB, patch
Details | Diff | Splinter Review
64.70 KB, patch
Details | Diff | Splinter Review
Now that we have support for post-mortem finalization in JS, we can consider auto-closing Sqlite.hsm databases upon garbage-collection.
mak, gps, do you think this would be useful/interesting?
Flags: needinfo?(mak77)
Flags: needinfo?(gps)
We have support for finalization in JS now?! Details!

When I originally wrote Sqlite.jsm, I had ideas for having connections automatically close after N seconds of inactivity so memory could be regained, etc. I'm all for implementing this!
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #2)
> We have support for finalization in JS now?! Details!

Well, not officially :)
But we have http://dxr.mozilla.org/mozilla-central/source/toolkit/components/finalizationwitness/nsIFinalizationWitnessService.idl

> When I originally wrote Sqlite.jsm, I had ideas for having connections
> automatically close after N seconds of inactivity so memory could be
> regained, etc. I'm all for implementing this!

Ok, I'll try and work on this when I find some time.
Assignee: nobody → dteller
as gps said, it was already an intended feature.
Flags: needinfo?(mak77)
I won't have time to handle this in the near future. If anyone wants to pick this bug, they're welcome.
Assignee: dteller → nobody
Assignee

Comment 6

5 years ago
Posted patch sqlitefinalize.patch (obsolete) — Splinter Review
Ok, I'll take a shot at this.  I've attached a patch, is it a step in the right direction?
Attachment #8369315 - Flags: feedback?(dteller)
Comment on attachment 8369315 [details] [diff] [review]
sqlitefinalize.patch

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

I believe that this can work. However, this is probably hard-to-maintain.

Let me suggest a refactoring that would have equivalent semantics but that I believe would be clearer:
- every instance of OpenedConnection should contain a field |_data| (or something such) that contains all the fields that are currently directly in that instance (you'll need to rewrite lots of code to make this happen, so that probably deserves one patch by itself);
- in |OpenedConnections|, save a reference to |_data|;
- add a field |_witness| or something such, directly in the instance, for the FinalizationWitness;
- for auto-closing, reconstruct a simili-|OpenedConnection| using |_data| and call |close()|.

::: toolkit/modules/Sqlite.jsm
@@ +157,5 @@
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {
> +  let connection = OpenConnections.extract(aValue);
> +
> +  // Close the connection
> +  // There probably should be some kind of warning logged here...

Good point.

@@ +161,5 @@
> +  // There probably should be some kind of warning logged here...
> +  try {
> +    connection._close();
> +  } catch (ex) {
> +    Cu.reportError("Error when auto-closing sqlite connection: " + ex);

Take a look at how the other functions in this module log errors. It's more powerful than just Cu.reportError.
Attachment #8369315 - Flags: feedback?(dteller) → feedback+
Assignee

Comment 8

5 years ago
Thanks for the suggestions.  Here I have attached one patch just for the refactoring part.

And here's the try results (just with the old commit name)
https://tbpl.mozilla.org/?tree=Try&rev=2d50ae74fb3b
Attachment #8369315 - Attachment is obsolete: true
Attachment #8371740 - Flags: feedback?(dteller)
Comment on attachment 8371740 [details] [diff] [review]
Refactor OpenedConnection to store information in |_data|

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

This looks good. Looking at the code, I realize that you should create a constructor ConnectionData for _data and the companion prototype. That prototype should define all the methods needed for closing the connection and the methods of OpenedConnection should dispatch to _data's methods for these methods.

Also, please document why you need this. Once this is done, ask :gps for a review, he's the owner of this code.
Attachment #8371740 - Flags: feedback?(dteller) → feedback+
Assignee

Comment 10

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #9)
> Comment on attachment 8371740 [details] [diff] [review]
> Refactor OpenedConnection to store information in |_data|
> 
> Review of attachment 8371740 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good. Looking at the code, I realize that you should create a
> constructor ConnectionData for _data and the companion prototype. That
> prototype should define all the methods needed for closing the connection
> and the methods of OpenedConnection should dispatch to _data's methods for
> these methods.
> 
> Also, please document why you need this. Once this is done, ask :gps for a
> review, he's the owner of this code.

Thanks, I'll do that.  Just to make sure I got this right, I need only to move the methods needed for closing into the ConnectionData prototype, so that I can easily auto-close it later if needed?  That is, I keep the other not-used-for-closing methods as they currently are in my patch, inside OpenedConnection and referring to instance data with this._data.<stuff>?

And the ConnectionData constructor should just encapsulate the initialization that's currently inside OpenedConnection constructor?
(In reply to Michael Brennan from comment #10)
> Thanks, I'll do that.  Just to make sure I got this right, I need only to
> move the methods needed for closing into the ConnectionData prototype, so
> that I can easily auto-close it later if needed?

Exactly. The idea is to auto-close the ConnectionData rather than the OpenedConnection. Once your work on this bug is complete, an instance of OpenedConnection will contain both an instance of ConnectionData (_data) and an instance of FinalizationWitness (_finalizer). Both the OpenedConnection and the _finalizer (with the help of your OpenedConnections data structure) will be able to trigger closing of the instance of ConnectionData. Keeping _finalizer out of _data ensures that we can keep a reference to _data without preventing _finalizer from being garbage-collected.

>  That is, I keep the other
> not-used-for-closing methods as they currently are in my patch, inside
> OpenedConnection and referring to instance data with this._data.<stuff>?

This seems like the simplest way to do it. :gps might ask you to rework this a little, we shall see.

> And the ConnectionData constructor should just encapsulate the
> initialization that's currently inside OpenedConnection constructor?

Indeed.
Assignee

Comment 12

5 years ago
Attachment #8371740 - Attachment is obsolete: true
Attachment #8375326 - Flags: review?(gps)
Comment on attachment 8375326 [details] [diff] [review]
Refactor OpenedConnection to use a OpenedData object

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

I would have called it this._connection, this._conn, or even this._c instead of this._data. But that's a bikeshed.

I like how you've encapsulated all the connection state/data into a self-contained object. That makes it much easier to throw state away and start with a new connection.

I didn't perform line-by-line verification that the supposedly copied code was copied without modifications. Should I be looking for any changes?

While I'm granting r+, I'd like to throw out a slightly alternative proposal. Instead of having OpenedConnection proxy methods via OpenedData, have you considered dynamically changing the .prototype of the connection? When the connection is open, the active prototype's execute() is defined to do the right thing. When the connection is closed, execute() will open a new connection, swap in the appropriate prototype (this.prototype = OpenedConnection), and call this.execute() (hitting the new prototype this time). I /think/ the current patch is pretty close to that - you just need to change some constructors around and set up prototype inheritance.

Again, that's just an idea. I'm still granting r+ (on the condition lines didn't change as part of the refactor).

::: services/metrics/tests/xpcshell/test_metrics_storage.js
@@ +98,5 @@
>  
>  add_task(function test_checkpoint_apis() {
>    let backend = yield Metrics.Storage("checkpoint_apis");
>    let c = backend._connection;
> +  let count = c._data._statementCounter;

The fact this file is accessing internal APIs and had to be changed as part of this patch is unfortunate. We should make statementCounter a public/proxied API. Follow-up bug.

::: toolkit/modules/Sqlite.jsm
@@ +597,5 @@
>     *
>     * @return Promise<>
>     */
>    close: function () {
> +    return this._data.close();

What if this._data is not defined? I guess I'll wait for a future patch to see!
Attachment #8375326 - Flags: review?(gps) → review+
Assignee

Comment 14

5 years ago
(In reply to Gregory Szorc [:gps] from comment #13)

Thanks for the review!

> I would have called it this._connection, this._conn, or even this._c instead
> of this._data. But that's a bikeshed.

There's no problem changing it. But _connection is already in use in the code, and it could be confusing to have them too similar. Do you want me to change the name of _connection to something else if I would change the name of _data, to _conn for instance?

> I like how you've encapsulated all the connection state/data into a
> self-contained object. That makes it much easier to throw state away and
> start with a new connection.
> 
> I didn't perform line-by-line verification that the supposedly copied code
> was copied without modifications. Should I be looking for any changes?

No, it's not necessary. I've only copied the code.

> While I'm granting r+, I'd like to throw out a slightly alternative
> proposal. Instead of having OpenedConnection proxy methods via OpenedData,
> have you considered dynamically changing the .prototype of the connection?
> When the connection is open, the active prototype's execute() is defined to
> do the right thing. When the connection is closed, execute() will open a new
> connection, swap in the appropriate prototype (this.prototype =
> OpenedConnection), and call this.execute() (hitting the new prototype this
> time). I /think/ the current patch is pretty close to that - you just need
> to change some constructors around and set up prototype inheritance.

Sounds good to try to avoid the proxying and simply use the correct prototype instead. But I got a bit confused from the example. You say that if we have a connection that is closed and call execute(), then it should open a new connection. Is that to add new functionality to be able to reopen an already closed connection? Or by opening a new connection, do you mean to create a new object on which we can change the prototype?

In either case, that would mean that we would only have one object for the connection, right? Where would we later put the finalization witness? The reference that we have to maintain in order to close on finalization would have to point to that object, and effectively prevent the connection object from being garbage collected. But I might just have misunderstood the idea here.
I also think _data and openedData looks too generic and confusing. openedConnection or connectionHandler may be fine... the internal _connection may be _dbConn, _rawConn or similarly named.
(In reply to Michael Brennan from comment #14)
> Sounds good to try to avoid the proxying and simply use the correct
> prototype instead. But I got a bit confused from the example. You say that
> if we have a connection that is closed and call execute(), then it should
> open a new connection. Is that to add new functionality to be able to reopen
> an already closed connection? Or by opening a new connection, do you mean to
> create a new object on which we can change the prototype?

Whatever you do, please make sure that you end up with the most readable solution. Going higher-order with prototypes just to avoid ~20loc is probably not a good investment.
We have the following states:

1) A "ready connection"
2) An "open connection"
3) A "closed connection"

A "ready connection" holds the necessary parameters to establish a connection.

I argue a "closed connection" can be modeled as an "open connection" that has transitioned into the "ready" state and is thus the same as a "ready connection." Although, we may wish to explicitly track a state that refuses to allow a closed connection to be reopened (say after shutdown has been initiated).

I think we can make things look like the following:

openConnection(options) -> ReadyConnection

ReadyConnection = function (options) {
  this._connOptions = options;
  this._connection = null;
  this._openAllowed = true;
}

ReadyConnection.prototype.execute = function (sql, params=null, onRow=null) {
  return this.open().then(() => {
    return this.execute(sql, params, onRow);
  });
};

ReadyConnection.prototype.open = function () {
  let deferred = Promise.defer();

  Services.storage.openAsyncDatabase(this._file, this._options, (status, connection) => {
    this._connection = connection;
    this.prototype = OpenedConnection;

    deferred.resolve(this);
  });

  return deferred;
};

OpenedConnection.prototype.execute = function (sql, params=null, onRow=null) {
  // Existing execute() function.
};

OpenedConnection.prototype.close = function () {
  // Existing close code.
  this._connection = null;
  this.prototype = ReadyConnection;

  return deferred;
};

We can either group all the "opened connection" variables inside a new object (like the existing patch), or we can leave them as member variables on the base object. I don't think it matters too much if you are doing dynamic prototypes.

I think this approach is more end-user friendly because it abstracts the complex logic of connection state management away from the user. They have a simple-to-use object that can be opened or closed (for performance reasons) without their knowledge or requirement to do anything special. That's extremely powerful and empowers us to change Sqlite.jsm without auditing all the callers.

What do you think of this approach?
Assignee

Comment 18

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #16)
> (In reply to Michael Brennan from comment #14)
> > Sounds good to try to avoid the proxying and simply use the correct
> > prototype instead. But I got a bit confused from the example. You say that
> > if we have a connection that is closed and call execute(), then it should
> > open a new connection. Is that to add new functionality to be able to reopen
> > an already closed connection? Or by opening a new connection, do you mean to
> > create a new object on which we can change the prototype?
> 
> Whatever you do, please make sure that you end up with the most readable
> solution. Going higher-order with prototypes just to avoid ~20loc is
> probably not a good investment.

I'll do my best :)
Assignee

Comment 19

5 years ago
I'm trying out your proposal and I'll attach a patch to show how far I got with it.

(In reply to Gregory Szorc [:gps] from comment #17)
> We can either group all the "opened connection" variables inside a new
> object (like the existing patch), or we can leave them as member variables
> on the base object. I don't think it matters too much if you are doing
> dynamic prototypes.

I think it would be a lot easier to have this data together like it is now to be able to auto-close it with the help of a finalization witness later.

> I think this approach is more end-user friendly because it abstracts the
> complex logic of connection state management away from the user. They have a
> simple-to-use object that can be opened or closed (for performance reasons)
> without their knowledge or requirement to do anything special. That's
> extremely powerful and empowers us to change Sqlite.jsm without auditing all
> the callers.
> 
> What do you think of this approach?

Sounds like a nice idea, but I don't see why dynamic prototypes are needed for this. If you look at my patch, it currently switches between the prototypes when opening and closing connections. So the set of methods exposed changes and thus the interface towards the user changes. To be sure to call the right methods the client would still have to track the state of the connection object, which we don't want. So maybe ReadyConnection is also intended to inherit from OpenedConnection? To keep the interface constant.

Right now there's three objects in play: ReadyConnection, OpenedConnection and OpenedData. OpenedData could renamed to make its intent more clear, but currently an instance of it is stored in _openedConnection when the connection is really open (the old _data member). I don't know if your suggestion also mean that OpenedData should be merged into OpenedConnection, to limit this to only two objects. But if that is the case, then I suppose we would have an instance of OpenedConnection stored in _openedConnection but at the same time the containing object would have a prototype of OpenedConnection, which I think would be quite confusing.

Perhaps I'm missing something vital here, but to me it seems easier to provide one interface towards the user in just a regular object, and keep the connection state management private. Then we could still have the connection management abstracted away from the user. What do you think?

Lastly, doesn't it look a bit weird to allow something like the following?

connection = openConnection({ path: somePath });
connection.open();  // wasn't it open already?

If we relieve the user from keeping track of the connection's state, is it too prohibitive to disallow the client from calling open? Otherwise, maybe some renaming could help?
Assignee

Comment 20

5 years ago
Added ReadyConnection. Renamed _connection to _dbConn, and _data to _openedConnection.
Attachment #8393047 - Flags: feedback?(gps)
Comment on attachment 8393047 [details] [diff] [review]
Tryout with dynamic prototypes

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

At a cursory glance, this looks good to me.

::: toolkit/modules/Sqlite.jsm
@@ +462,3 @@
>  }
>  
> +function ReadyConnection(options, path, openedOptions) {

Documentation needed.

@@ +479,5 @@
> +  open: function () {
> +    let deferred = Promise.defer();
> +    let log = Log.repository.getLogger("Sqlite.ConnectionOpener");
> +
> +    let file = FileUtils.File(this._path);

Actually, we don't need |file|. Just passing |this._path| to |openAsyncDatabase| should work nicely.

@@ +626,5 @@
>     *
>     * @return Promise<>
>     */
>    close: function () {
> +    var promise = this._openedConnection.close();

Please use let instead of var.

@@ +628,5 @@
>     */
>    close: function () {
> +    var promise = this._openedConnection.close();
> +    this._openedConnection = null;
> +    this.__proto__ = ReadyConnection.prototype;

This needs careful documentation.
Attachment #8393047 - Flags: feedback+
So, prototype mutation like this is Bad.  It's slow initially, and you forever taint property access speed on that object, and objects along its prototype chain.  Don't do it!

In this case, if I read the patch correctly, you have connection objects.  They contain a _dbConnection field that's (presumably) connection info.  And that's it.  And you have a prototype with methods that act on that connection info.  And the only reason -- I think -- that you have prototype mutation here, is because you think it gets rid of overhead to test whether the connection is open on each method-call?

First, I would question whether the overhead of such a test is so bad.  If it's just checking the value of a single integer/boolean property of this._dbConnection or so, it's not going to amount to enough win overall to justify other approaches.  It's not going to overcome the cost of prototype mutation, both initially and on an ongoing basis.

If the test really is sort of bad -- which, honestly, seems unlikely when there's such a clear state-change here -- I'd probably have a single field that's a "connection implementation" object.  Then your Connection.prototype.foo() method could just forward to |this._impl.foo()|, and you wouldn't have an extra check.  The method-forwarding shouldn't cost much once the JITs touch this...I hope.  I'm not sure that totally-plain forwarding of arguments and method calls will translate with utmost optimality.

All things considered, I'd just have the connection methods check a field to know current state and leave it at that.  Prototype mutation isn't worth it for this.
Jeff: What you say is true. But I thought the discussion on dev-platform that brought you here eventually concluded that the overhead is not significant. It's on the order of 20 cycles or something. It really only impacts heavily JIT optimized code. Etc. Chrome JS probably won't feel it. Am I wrong?
Comment on attachment 8393047 [details] [diff] [review]
Tryout with dynamic prototypes

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

I like the approach!
Attachment #8393047 - Flags: feedback?(gps) → feedback+
I believe that prototype mutation:
- makes things harder to follow for most potential contributors;
- will cause a warning to be displayed, which is bad for debugging.

I therefore vote against prototype mutation.
Assignee

Comment 26

5 years ago
How shall I continue with this issue?
If I were to choose I'd skip mutating the prototype mostly because of readability, but I am fine continuing with that approach also, if we decide to go on with it.
For the first version, don't mutate the prototype. If gps believe that mutating it would really improve code quality, let's evaluate that in a followup bug.
Assignee

Comment 28

5 years ago
Essentially same as previous version, except some updated names and merged with newer changes in the tree.
Attachment #8375326 - Attachment is obsolete: true
Attachment #8422737 - Flags: review?(gps)
Comment on attachment 8422737 [details] [diff] [review]
sqlitefinalize-part1, refactorization

I believe that gps is on sabbatical. Taking over the review.
Attachment #8422737 - Flags: review?(gps) → review?(dteller)
Comment on attachment 8422737 [details] [diff] [review]
sqlitefinalize-part1, refactorization

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

Quick remark: a non-trivial patch to Sqlite.jsm has landed yesterday. It affects opening and closing. You should make sure to update your patch so that it still works after these changes.
Attachment #8422737 - Flags: review?(dteller)
Whiteboard: [Async][mentor=Yoric][lang=js]
Assignee

Comment 31

5 years ago
I haven't added any tests. I could add a test that sends a notification and asserts that the connection is really closed, but I believe that I then need to add a promise that is resolved when the database is auto-closed, and also expose some internal data to the test.

I tried commenting out a close() in firefox and ran it. Quite often I get a crash since the database doesn't seem to close before it's too late. I suppose we could add an async shutdown blocker to solve that, but maybe it's easy to run into some condition where the blocker waits but the database doesn't get garbage collected.

What do you think about the above two issues, are they worth implementing?
Attachment #8393047 - Attachment is obsolete: true
Attachment #8422745 - Flags: feedback?(dteller)
Assignee

Comment 32

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on May 8th-12th) from comment #30)
> Comment on attachment 8422737 [details] [diff] [review]
> sqlitefinalize-part1, refactorization
> 
> Review of attachment 8422737 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Quick remark: a non-trivial patch to Sqlite.jsm has landed yesterday. It
> affects opening and closing. You should make sure to update your patch so
> that it still works after these changes.

Oh, thanks for the heads up. I'll take a look at it.
Comment on attachment 8422745 [details] [diff] [review]
sqlitefinalize-part2, auto-close

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

Looks good, with minor nits. Now, I'd like tests!

::: toolkit/modules/Sqlite.jsm
@@ +33,5 @@
>  let connectionCounters = new Map();
>  
> +// Used by finalization witnesses to be able to close opened
> +// connections on garbage collection.
> +let connIdToConnData = new Map();

Please document the keys and values.

@@ +35,5 @@
> +// Used by finalization witnesses to be able to close opened
> +// connections on garbage collection.
> +let connIdToConnData = new Map();
> +
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {

Some more comments would be useful.

@@ +38,5 @@
> +
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {
> +  let connectionIdentifier = aValue;
> +  let connectionData = connIdToConnData.get(connectionIdentifier);
> +  connIdToConnData.delete(aValue);

That would be connectionIdentifier.

@@ +41,5 @@
> +  let connectionData = connIdToConnData.get(connectionIdentifier);
> +  connIdToConnData.delete(aValue);
> +
> +  dump("Warning: Sqlite connection '" + connectionIdentifier +
> +       "' was not properly closed. Auto-close triggered by garbage collection.\n");

Let's use `Services.console` instead of `dump`. This will make it easier to test the property, as we can add observers.

@@ +43,5 @@
> +
> +  dump("Warning: Sqlite connection '" + connectionIdentifier +
> +       "' was not properly closed. Auto-close triggered by garbage collection.\n");
> +
> +  connectionData.close();

This will fail if it gets executed too late during shutdown. We will need to find a solution or at least handle this case through better error reporting.

@@ +614,5 @@
>    // field without preventing garbage collection of
>    // OpenedConnection. On garbage collection, we will still be able to
>    // close the database using this extra reference.
> +  this._openedConnection = new ConnectionData(connection, basename,
> +                                              number, options);

Let's not repaginate needlessly.
Attachment #8422745 - Flags: feedback?(dteller) → feedback+
Assignee

Comment 34

5 years ago
Updated and merged with updates in tree.
Attachment #8422737 - Attachment is obsolete: true
Attachment #8428330 - Flags: feedback?(dteller)
Assignee

Comment 35

5 years ago
Tests added and changes suggested from feedback applied.
Attachment #8422745 - Attachment is obsolete: true
Attachment #8428331 - Flags: feedback?(dteller)
Assignee

Comment 36

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on May 16th-23th) from comment #33)
> @@ +43,5 @@
> > +
> > +  dump("Warning: Sqlite connection '" + connectionIdentifier +
> > +       "' was not properly closed. Auto-close triggered by garbage collection.\n");
> > +
> > +  connectionData.close();
> 
> This will fail if it gets executed too late during shutdown. We will need to
> find a solution or at least handle this case through better error reporting.
> 

I have not done anything for this one. Should we try to solve it here or leave it for a follow up bug?
Comment on attachment 8428330 [details] [diff] [review]
sqlitefinalize-part1, refactorization v2

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

Looks good to me, with the minor changes below.
As far as I'm concerned, if tests pass, this can get r+. mak, do you want to perform an additional review?

::: toolkit/modules/Sqlite.jsm
@@ +102,5 @@
>  /**
> + * Connection data with methods necessary for closing the connection.
> + *
> + * To support auto-closing in the event of garbage collection, this
> + * type contains all the connection data of an opened connection and

Nit: It's not a type, it's a data structure or a constructor.

@@ +154,5 @@
> +    this._deferredClose.promise
> +  );
> +}
> +
> +ConnectionData.prototype = Object.freeze({

I haven't checked every single line of this proto. Can you confirm that this is a simple cut-paste from OpenConnection.prototype?

@@ +678,5 @@
> +  // witness. This enables us to store an additional reference to this
> +  // field without preventing garbage collection of
> +  // OpenedConnection. On garbage collection, we will still be able to
> +  // close the database using this extra reference.
> +  this._openedConnection = new ConnectionData(connection, basename, number, options);

It's a bit weird to have a field `_openedConnection` in a constructor `OpenedConnection`. I'd call this `_connectionData`.

@@ +752,5 @@
>     *        the original connection.
>     *
>     * @return Promise<OpenedConnection>
>     */
>    clone: function (readOnly=false) {

I don't really like to see us hitting in a private field of another constructor (here, `_log`). I would forward this to a method `ConnectionData.prototype.clone`.

@@ +828,2 @@
>     */
>    executeCached: function (sql, params=null, onRow=null) {

I would forward this to a method `ConnectionData.prototype.executeCached`.

@@ +884,5 @@
>    /**
>     * Whether a transaction is currently in progress.
>     */
>    get transactionInProgress() {
> +    return this._openedConnection._open && !!this._openedConnection._inProgressTransaction;

Same here and for other methods below.
Attachment #8428330 - Flags: feedback?(dteller) → review+
Attachment #8428330 - Flags: review?(mak77)
Comment on attachment 8428331 [details] [diff] [review]
sqlitefinalize-part2, auto-close v2

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

This looks good.
We'll need to also handle the case in which garbage-collection takes place too late, after the shutdown barrier, in which case we end up with a shutdown freeze/crash. That can wait for a followup bug, though.

::: toolkit/modules/Sqlite.jsm
@@ +45,5 @@
> + *
> + * Key: _connectionIdentifier of ConnectionData
> + * Value: ConnectionData object
> + */
> +let connIdToConnData = new Map();

For the sake of encapsulation, I would make this a field of constructor `ConnectionData` itself. This will let you rename it to something a little more readable, e.g. `byId`.

@@ +119,5 @@
> + *
> + * The observer is passed the connection identifier of the database
> + * connection that is being finalized.
> + */
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {

This call to `Services` would force Services.jsm to be imported during startup, which is something we want to avoid. Could you make this call to `addObserver` in the initialization of `Barrier`, for instance? Alternatively, make field `ConnectionData.byId` a lazy getter and add this observer during the initialization of `byId`.

Also, nit: in this file, we don't prefix arguments with "a". That's a C++-ism that we try to avoid in recent JS code (not always successfully).

@@ +120,5 @@
> + * The observer is passed the connection identifier of the database
> + * connection that is being finalized.
> + */
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {
> +  let connectionIdentifier = aValue;

Nit: Let's just call the argument `connectionIdentifier`, in that case.

@@ +122,5 @@
> + */
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {
> +  let connectionIdentifier = aValue;
> +  let connectionData = connIdToConnData.get(connectionIdentifier);
> +  connIdToConnData.delete(connectionIdentifier);

Is there a path that could lead to `connectionIdentifier` not being in the map?

@@ +126,5 @@
> +  connIdToConnData.delete(connectionIdentifier);
> +
> +  Services.console.logStringMessage(
> +    "Warning: Sqlite connection '" + connectionIdentifier +
> +    "' was not properly closed. Auto-close triggered by garbage collection.\n");

Could you make this a script error message?
See e.g. http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/AddonLogging.jsm#89-96

And let's follow that example and put back the `dump` I asked you to remove, as a fallback.

Also, could you add a comment somewhere mentioning that `connectionIdentifier` contains the human-readable name of the database?

@@ +618,5 @@
>      throw new TypeError("Connection must be a valid Storage connection.");
>    }
>  
>    if (isClosed) {
> +    throw new Error("Sqlite.jsm has been shutdown. Cannot clone connection to: " + source.database.path);

Ah, thanks for the typo fix.

@@ +718,5 @@
> +  // Store the extra reference in a map with connection identifier as
> +  // key. Then create the finalization witness.
> +  connIdToConnData.set(this._openedConnection._connectionIdentifier,
> +                       this._openedConnection);
> +  this._witness = FinalizationWitnessService.make(

This field deserves more documentation.

@@ +780,5 @@
>    close: function () {
> +    // An entry in the map indicates we have an active witness. Since
> +    // we are closing, tell the witness to forget us. This condition
> +    // should always evaluate to true unless close is called more than
> +    // once.

I'd prefer a slightly shorter line. Can you call the id `id`?

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +928,5 @@
> +  let warningLogged = false;
> +  let listener = {
> +    observe: function(aMessage) {
> +      let messageText = aMessage.message;
> +      if (messageText.indexOf("Warning: Sqlite connection") !== -1) {

Could you make sure that the connectionIdentifier also appears in the message?

@@ +948,5 @@
> +
> +  do_check_true(warningLogged);
> +});
> +
> +add_task(function test_forget_witness_on_close() {

function*

@@ +963,5 @@
> +
> +  yield c.close();
> +  // After close, witness should have forgotten the connection
> +  do_check_true(forgetCalled);
> +});

Now, I'd like a test about garbage-collection. Since gc is not an exact science, I would go for something along the lines of:

add_task(function* foo() {

  let deferred = Promise.defer();

  for (let i = 0; i < 100; ++i) {
    let c = yield getDummyDatabase(...);

    // Release `deferred` once one of the databases has been closed by the gc
    // (any of them will do).
    c._openedConnection._deferredClose.promise.then(deferred.promise.resolve); 
  }

  Components.utils.forceGC();
  yield deferred.promise;
});
Attachment #8428331 - Flags: feedback?(dteller) → feedback+
Comment on attachment 8428330 [details] [diff] [review]
sqlitefinalize-part1, refactorization v2

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

I think the review job has been very good here so far, I don't have any important points to bring out, so I'll let the final pass to David who greatly managed it so far.

I'm just pointing out some nits about very simple cleanup that we may do while we are in the process of movi this code (some or most are not related to the changes we are making, but sounds like a good time for trivial cleanup)

::: toolkit/modules/Sqlite.jsm
@@ +119,5 @@
> +function ConnectionData(connection, basename, number, options) {
> +  this._log = Log.repository.getLoggerWithMessagePrefix("Sqlite.Connection." + basename,
> +                                                        "Conn #" + number + ": ");
> +
> +  this._log.info("Opened");

nit: remove the newline above this.

@@ +258,5 @@
> +    this._open = false;
> +
> +    this._log.debug("Calling asyncClose().");
> +    this._dbConn.asyncClose({
> +      complete: function () {

please, while moving this, make it:

this._dbConn.asyncClose(() => {
  ...
});

first of all mozIStorageCompletionCallback has the function decorator (so it doesn't need to be an object with a complete method), second arrow function allows you to avoid the .bind(this)

@@ +286,5 @@
> +    let onFinished = function () {
> +      this._anonymousStatements.delete(index);
> +      statement.finalize();
> +      this._startIdleShrinkTimer();
> +    }.bind(this);

also, please make this an arrow function, so you can remove .bind(this)

@@ +297,5 @@
> +          onFinished();
> +          deferred.resolve(rows);
> +        }.bind(this),
> +
> +        function onError(error) {

as well as here we may have

rows => {...},
error  => {...}

(regardless I don't see why we are using .bind(this) on these 2 functions, this is not even used and onFinished is already bound)

@@ +315,5 @@
> +    this._log.info("Shrinking memory usage.");
> +
> +    let onShrunk = this._clearIdleShrinkTimer.bind(this);
> +
> +    return this.execute("PRAGMA shrink_memory").then(onShrunk, onShrunk);

nit: I don't see why there are so many newlines here :)
Attachment #8428330 - Flags: review?(mak77) → feedback+
Comment on attachment 8428331 [details] [diff] [review]
sqlitefinalize-part2, auto-close v2

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

Apart from David's comments that look sane:

::: toolkit/modules/Sqlite.jsm
@@ +119,5 @@
> + *
> + * The observer is passed the connection identifier of the database
> + * connection that is being finalized.
> + */
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {

I think the topic is expected just once, so the observer should be removed on first use?
Assignee

Comment 41

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #37)
> @@ +154,5 @@
> > +    this._deferredClose.promise
> > +  );
> > +}
> > +
> > +ConnectionData.prototype = Object.freeze({
> 
> I haven't checked every single line of this proto. Can you confirm that this
> is a simple cut-paste from OpenConnection.prototype?

Yes, the methods are unmodified with the exception of some renamings of `_connection` to `_dbConn`.

> @@ +678,5 @@
> > +  // witness. This enables us to store an additional reference to this
> > +  // field without preventing garbage collection of
> > +  // OpenedConnection. On garbage collection, we will still be able to
> > +  // close the database using this extra reference.
> > +  this._openedConnection = new ConnectionData(connection, basename, number, options);
> 
> It's a bit weird to have a field `_openedConnection` in a constructor
> `OpenedConnection`. I'd call this `_connectionData`.

Ah, I found this myself some time ago and thought it was weird. I also thought I had fixed it, but apparently not :-) Thanks for noticing!

> 
> @@ +828,2 @@
> >     */
> >    executeCached: function (sql, params=null, onRow=null) {
> 
> I would forward this to a method `ConnectionData.prototype.executeCached`.
> 
> @@ +884,5 @@
> >    /**
> >     * Whether a transaction is currently in progress.
> >     */
> >    get transactionInProgress() {
> > +    return this._openedConnection._open && !!this._openedConnection._inProgressTransaction;
> 
> Same here and for other methods below.


While fixing the remaining methods by forwarding methods calls to `ConnectionData` I ran into a problem where I need some help.

According to the documentation comment, `OpenedConnection.prototype.executeTransaction` takes a callback which, when called, will be passed the connection instance, which here is the `OpenedConnection` instance. So if this method is moved into `ConnectionData.prototype.executeTransaction` it will need a reference to the `OpenedConnection` instance for it to be able to call the callback with the right argument.

But passing `this` when calling `ConnectionData.prototype.executeTransaction` sounds really bad. We don't want a reference back to the `OpenedConnection` preventing garbage collection. Maybe the lifetime of the closure is short enough to pose no problem, I haven't tried yet, but to me this looks like something we should really try to avoid.

What are your opinion on this?
Flags: needinfo?(dteller)
Good question.
Indeed, we don't want to pass the instance of `ConnectionData` nor to have a backref. So we need to make sure that `func` knows about the instance of `OpenedConnection` by the time it reaches `ConnectionData.prototype.executeTransaction`, as follows:

OpenedConnection.prototype.executeTransaction = function(func, ...) {
  return this._connectionData.executeTransaction(() => func(this), ...);
}
Flags: needinfo?(dteller)
Assignee

Comment 43

5 years ago
(In reply to Marco Bonardo [:mak] from comment #40)
> Comment on attachment 8428331 [details] [diff] [review]
> sqlitefinalize-part2, auto-close v2
> 
> Review of attachment 8428331 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Apart from David's comments that look sane:
> 
> ::: toolkit/modules/Sqlite.jsm
> @@ +119,5 @@
> > + *
> > + * The observer is passed the connection identifier of the database
> > + * connection that is being finalized.
> > + */
> > +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {
> 
> I think the topic is expected just once, so the observer should be removed
> on first use?

It will be called for every connection that gets garbage-collected, so it should stay until the end. But thanks for the reminder, I've added a removeObserver at shutdown.
Assignee

Comment 44

5 years ago
Attachment #8428330 - Attachment is obsolete: true
Assignee

Comment 45

5 years ago
Attachment #8428331 - Attachment is obsolete: true
Attachment #8438166 - Flags: feedback?(dteller)
Comment on attachment 8438166 [details] [diff] [review]
sqlitefinalize-part2, auto-close v3

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

I like that patch a lot. I'll be happy to r+ it once you have finished the other part.

::: toolkit/modules/Sqlite.jsm
@@ +50,5 @@
> +  consoleMessage.init(message, stack.fileName, null, stack.lineNumber, 0,
> +                      Ci.nsIScriptError.errorFlag, "component javascript");
> +  Services.console.logMessage(consoleMessage);
> +
> +  // Always dump errors, in case the Console Service isn't listening yet

Nit: "or isn't listening anymore".

@@ +119,5 @@
>        // Now, wait until all databases are closed
>        yield Barriers.connections.wait();
> +
> +      // Everything closed, no finalization events to catch
> +      Services.obs.removeObserver(finalizationObserver, "sqlite-finalization-witness");

Good idea.

@@ +938,5 @@
>     * @return Promise<>
>     */
>    close: function () {
> +    // Unless cleanup has already been done by a previous call to
> +    // `close', delete the database entry from map and tell the

Nit: Maybe `close` (rather than `close'), for consistency?

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +996,5 @@
> +  // references. This is needed at the moment, otherwise
> +  // garbage-collection takes place after the shutdown barrier and the
> +  // test will timeout. Once that is fixed, we can remove this line
> +  // and be fine as long as at least one connection is
> +  // garbage-collected.

Really? That's sad.
Attachment #8438166 - Flags: feedback?(dteller) → feedback+
Assignee

Comment 47

5 years ago
Thank you David.
I've applied changes from feedback. Adding review flag to both patches and hope everything looks alright.
Attachment #8438165 - Attachment is obsolete: true
Attachment #8438738 - Flags: review?(dteller)
Assignee

Comment 48

5 years ago
Attachment #8438166 - Attachment is obsolete: true
Attachment #8438740 - Flags: review?(dteller)
Comment on attachment 8438738 [details] [diff] [review]
sqlitefinalize-part1, refactorization v3

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

Could you double-check the `func` (see below)? Other than that, this seems good to me, with minor nits.

::: toolkit/modules/Sqlite.jsm
@@ +271,5 @@
>        this._startIdleShrinkTimer();
>        throw ex;
>      }
>  
>      return deferred.promise;

All this block (from `let deferred` to the final `return`) could be rewritten as follows:

return Task.spawn(function*() {
  try {
    return yield this._executeStatement(sql, statement, params, onRow);
  } finally {
    this._startIdleShrinkTimer();
  }
}.bind(this));

Which would also provide better stack backtraces in case of error. Your call, you can either do it now or keep it for a followup bug.

@@ +311,5 @@
>        onFinished();
>        throw ex;
>      }
>  
>      return deferred.promise;

Same here.

@@ +337,5 @@
>        yield this.execute("BEGIN " + type + " TRANSACTION");
>  
>        let result;
>        try {
> +        result = yield Task.spawn(func);

Shouldn't this be `func()`?

@@ +342,5 @@
>        } catch (ex) {
>          // It's possible that a request to close the connection caused the
>          // error.
> +        // Assertion: close() will unset
> +        // this._inProgressTransaction when called.

Please don't repaginate.

@@ +510,5 @@
>        },
>  
>        handleError: function (error) {
> +        self._log.info("Error when executing SQL (" +
> +                       error.result + "): " + error.message);

Please don't repaginate.
Attachment #8438738 - Flags: review?(dteller) → review+
Comment on attachment 8438740 [details] [diff] [review]
sqlitefinalize-part2, auto-close v4

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

Looks good, thanks.

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +986,5 @@
> +
> +add_task(function* test_close_database_on_gc() {
> +  let deferred = Promise.defer();
> +
> +  for (let i = 0; i < 100; ++i) {

Can you add a comment that we do it 100 times to encourage the gc to collect at least one of the instances?
Attachment #8438740 - Flags: review?(dteller) → review+
Comment on attachment 8438740 [details] [diff] [review]
sqlitefinalize-part2, auto-close v4

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

::: toolkit/modules/Sqlite.jsm
@@ +51,5 @@
> +                      Ci.nsIScriptError.errorFlag, "component javascript");
> +  Services.console.logMessage(consoleMessage);
> +
> +  // Always dump errors, in case the Console Service isn't listening anymore
> +  dump("*** " + message + "\n");

Last minute idea.
Instead of dump(...), you should call `Promise.reject(new Error(message));`. In addition to displaying the error message, it will also cause unit tests to fail if we reach this line.

Note: If this idea causes tests to fail immediately, we can keep it for a followup bug.
(In reply to David Rajchenbach Teller [:Yoric] from comment #38)
> > +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {
> 
> This call to `Services` would force Services.jsm to be imported during
> startup, which is something we want to avoid.

In general this is good advice, but Services.jsm specifically is likely already loaded before anyone invokes Sqlite.jsm, so I doubt this matters in practice, right?
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #53)
> In general this is good advice, but Services.jsm specifically is likely
> already loaded before anyone invokes Sqlite.jsm, so I doubt this matters in
> practice, right?

Well, the two reasons are:
1. we have rewritten some of Sqlite.jsm specifically to import as little as possible during startup, so let's not waste the effort;
2. bug 859912.

But yes, here, it's not a big deal.
Any chance you could wrap this up quickly, Michael?
Assignee: nobody → brennan.brisad
Flags: needinfo?(brennan.brisad)
Mentor: dteller
Whiteboard: [Async][mentor=Yoric][lang=js] → [Async][lang=js]
Assignee

Comment 56

5 years ago
I've folded the patches with the suggestions from the review added, including changing `func` to `func()` in the task spawning.

However, `Promise.reject(new Error(message));` caused the tests to fail so I left the `dump` unchanged.

Shall I add the flag for checkin-needed?
Flags: needinfo?(brennan.brisad)
Yes, please do.
Don't forget to paste a link to your latest (successful) Try run.
Assignee

Comment 58

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=1a2650fd2437

Those Marionette-tests that fail don't seem to have been there on my previous Try run. I don't think they are related to my change, but they don't seem intermittent either. How should I proceed here?
Flags: needinfo?(dteller)
Looks like an infrastructure problem, indeed. Let's mark this bug as checkin-needed.
Flags: needinfo?(dteller)
Assignee

Updated

5 years ago
Keywords: checkin-needed
Those Marionette failures weren't there on your previous Try run because they were only recently enabled on Windows. I've retriggered them, but that sure looks suspicious to me.

And FWIW, consistent failures across all opt/debug builds of the same platform generally doesn't say "infrastructure" to me ;)
Keywords: checkin-needed
My only thought on the failures is that maybe you pushed against an old head (rather than mozilla-central tip) and you picked up an incidental test failure along the way (since there were some failures that needed fixing before Mn could be enabled on Windows). A good reason to always make sure your Try pushes are on top of m-c tip, FWIW ;).

Anyway, please do a push on top of m-c tip with the below syntax to verify that things are OK:
try: -b do -p win32 -u marionette -t none
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b018c57be4cb
https://hg.mozilla.org/mozilla-central/rev/7ccab6cb126f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Blocks: 1098965
You need to log in before you can comment on or make changes to this bug.