702815 - Maintain a list of open SQLite connections

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 687726 and bug 700508 both need a list of open SQLite connections, for different purposes.  This shouldn't be hard to add.  The existing patch in bug 687726 is a good start.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch

Simple patch.  I don't like getConnections() much but I'm not aware of a better way to allow an nsTArray to be iterated over.

I haven't written a test because I couldn't think of how to write one.  But I have written a patch for bug 703143 on top of this, and it's working, so there's some testing there.

Comment 2

[Marco Bonardo [:mak]]

As a side note, I was thinking if would make sense to use nsTObserverArray that gives you a safer enumerator to walk it.
I think we may also have a thread-safe vector implementation in the codebase? not sure about that though.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 575034 [details] [diff] [review]
patch

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

In general, this is fine.  I'm r-ing this only for the thread safety issues.

::: storage/src/mozStorageService.cpp
@@ +280,5 @@
>  Service::Service()
>  : mMutex("Service::mMutex"),
> +  mSqliteVFS(nsnull),
> +  mRegistrationMutex("Service::mRegistrationMutex"),
> +  mConnections()

I don't know how `mSqliteVFS` snuck in, but Storage style is to put the comma under the :.  hg.mozilla.org/mozilla-central/annotate/7dd46087e678/storage/src/mozStorageAsyncStatementExecution.h#l120

@@ +310,5 @@
>  }
>  
>  void
> +Service::registerConnection(Connection *aConnection)
> +{

`mRegistrationMutex.AssertNotCurrentThreadOwns();` here please

@@ +312,5 @@
>  void
> +Service::registerConnection(Connection *aConnection)
> +{
> +  MutexAutoLock mutex(mRegistrationMutex);
> +  mConnections.AppendElement(aConnection);

`(void)` return value because we are not checking it please.

@@ +317,5 @@
> +}
> +
> +void
> +Service::unregisterConnection(Connection *aConnection)
> +{

`mRegistrationMutex.AssertNotCurrentThreadOwns();` here please

@@ +320,5 @@
> +Service::unregisterConnection(Connection *aConnection)
> +{
> +  MutexAutoLock mutex(mRegistrationMutex);
> +  bool removed = mConnections.RemoveElement(aConnection);
> +  NS_ASSERTION(removed, "Tried to unregister a non-existent connection");

Use `MOZ_ASSERT` so it's actually fatal here, and move the text to a comment above the assert (and change it so it makes more sense in comment form I guess).

::: storage/src/mozStorageService.h
@@ +110,5 @@
> +   * can be iterated over.
> +   *
> +   * @param  aConnection
> +   *         The connection to register.
> +   */

Instead of stating which mutex will be used, I'd rather use the precondition syntax like we use elsewhere in storage:
http://hg.mozilla.org/mozilla-central/annotate/7dd46087e678/storage/src/mozStorageAsyncStatementExecution.h#l120

@@ +127,5 @@
> +   * Gets the list of open connections.
> +   *
> +   * @return The open connections.
> +   */
> +  nsTArray<Connection *> *getConnections();

This isn't going to be safe.  Consumers will be given a pointer to an array that could then mutate out from under them.  We either need to (a) copy the array, or (b) provide some way from them to lock it, which seems like a footgun.  (a) should be cheap, so let's just do that.

@@ +150,5 @@
> +
> +  /**
> +   * The list of connections we have created.
> +   */
> +  nsTArray<Connection*> mConnections;

nit: Storage style is `Connection *`, and I'd like to not mix them.
http://hg.mozilla.org/mozilla-central/annotate/ac667309bea6/storage/style.txt#l45

Comment 4

[Shawn Wilsher :sdwilsh]

(In reply to Shawn Wilsher :sdwilsh from comment #3)
> @@ +127,5 @@
> > +   * Gets the list of open connections.
> > +   *
> > +   * @return The open connections.
> > +   */
> > +  nsTArray<Connection *> *getConnections();
> 
> This isn't going to be safe.  Consumers will be given a pointer to an array
> that could then mutate out from under them.  We either need to (a) copy the
> array, or (b) provide some way from them to lock it, which seems like a
> footgun.  (a) should be cheap, so let's just do that.
Oh, (a) isn't quite good enough either because we could hand out a garbage pointer.  I guess we need to give back an `nsTArray<nsWeakRef<Connection> >`.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Shawn Wilsher :sdwilsh from comment #4)
> Oh, (a) isn't quite good enough either because we could hand out a garbage
> pointer.  I guess we need to give back an `nsTArray<nsWeakRef<Connection> >`.

nsWeakRef isn't threadsafe either.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: patch v2

Seems to me that locking the connections list before iterating is the safest thing -- we don't want another thread adding or removing any connections in the middle of iteration.

So I added getAndLockConnectionsList() and unlockConnectionsList().  I realize that MutexAutoLock is greatly preferred, but to do that here I'd instead have to create iterateOverConnections() which would take a function and a closure, and the lock/unlock pair seemed preferable, esp. since if you forget to unlock the resulting deadlock will be extremely obvious (I confirmed that with the patch from bug 703143).

I fixed the smaller problems too.

Comment 7

[Marco Bonardo [:mak]]

did anybody consider comment 2? nsTObserverArray is safe to use when the array changes while you loop it.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> So I added getAndLockConnectionsList() and unlockConnectionsList().  I
> realize that MutexAutoLock is greatly preferred, but to do that here I'd
> instead have to create iterateOverConnections() which would take a function
> and a closure, and the lock/unlock pair seemed preferable, esp. since if you
> forget to unlock the resulting deadlock will be extremely obvious (I
> confirmed that with the patch from bug 703143).
I don't immediately like this, but I'm going to sleep on it and get back with you in about 24 hours.

(In reply to Marco Bonardo [:mak] from comment #7)
> did anybody consider comment 2? nsTObserverArray is safe to use when the
> array changes while you loop it.
I'll also look at this.

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to Marco Bonardo [:mak] from comment #7)
> did anybody consider comment 2? nsTObserverArray is safe to use when the
> array changes while you loop it.

It's not threadsafe!  So it won't work here.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We should think about this a little... We basically have the same problem with SQLite connections that I had with workers (trying to synchronously grab the memory usage from multiple threads that we don't control).

Rather than make some API that will be difficult to use (getAndLockConnectionsList() and unlockConnectionsList()) or some API that will be ineffective (getConnections()) I think you should probably just add a LockAndReportMemory function that does what you want and nothing else.

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to ben turner [:bent] from comment #10)
> We should think about this a little... We basically have the same problem
> with SQLite connections that I had with workers (trying to synchronously
> grab the memory usage from multiple threads that we don't control).
> 
> Rather than make some API that will be difficult to use
> (getAndLockConnectionsList() and unlockConnectionsList()) or some API that
> will be ineffective (getConnections()) I think you should probably just add
> a LockAndReportMemory function that does what you want and nothing else.

This list of connections is needed for bug 411894 and bug 687726 as well, so we need a general-purpose facility.

Is getAndLock/unlock really a difficult to use API?  It doesn't seem that bad to me, but I can do an iterateOverConnections(func, closure) style API if people prefer that.

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to ben turner [:bent] from comment #5)
> nsWeakRef isn't threadsafe either.
Can you elaborate?  I thought we used threadsafe refcounting, so as long as we get a strong reference before we use the connection, we'd be OK.

(In reply to Nicholas Nethercote [:njn] from comment #11)
> Is getAndLock/unlock really a difficult to use API?  It doesn't seem that
> bad to me, but I can do an iterateOverConnections(func, closure) style API
> if people prefer that.
Yeah, I decided I don't like the API in your last patch for the following reasons:
1) it depends on reviewers to make sure that we always call the unlock function.  I'm mostly worried about this in the possible handling of error conditions that might be difficult to test or just miss testing.
2) the API feels awkward and is unlike other parts of storage

I'd much rather you just copy the array while you lock it, and maybe return strong references pending bent's answer.

Comment 13

[Nicholas Nethercote [inactive]]

> I'd much rather you just copy the array while you lock it, and maybe return
> strong references pending bent's answer.

What about the |iterateOverConnections(func, closure)| approach?  That would lock before and unlock after iterating, but because the locking is encapsulated so you can't forget to unlock.  I don't understand why weaf references would be needed.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Shawn Wilsher :sdwilsh from comment #12)
> Can you elaborate?  I thought we used threadsafe refcounting, so as long as
> we get a strong reference before we use the connection, we'd be OK.

Nothing in nsWeakReference.cpp is threadsafe (not just the addref/release impl, but the whole mechanism for nulling out its referent).

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> What about the |iterateOverConnections(func, closure)| approach?  That would
> lock before and unlock after iterating, but because the locking is
> encapsulated so you can't forget to unlock.  I don't understand why weaf
> references would be needed.
We could easily introduce code that is long running in that callback, so similar issues as (1) above.  Let's just return an array of strong references to the connections.  That's the simplest and safest route.

(In reply to ben turner [:bent] from comment #14)
> Nothing in nsWeakReference.cpp is threadsafe (not just the addref/release
> impl, but the whole mechanism for nulling out its referent).
Sadfaces, but I guess that makes sense.

Comment 16

[Nicholas Nethercote [inactive]]

> We could easily introduce code that is long running in that callback, so
> similar issues as (1) above.  Let's just return an array of strong
> references to the connections.  That's the simplest and safest route.

I don't think it's safe;  it's horribly racy.  The strong references ensure that no Connection is destroyed while multiple threads are doing their thing, but you could have multiple threads reading and writing Connections at the same time, and you could also have one thread destroy the underlying SQLite connection while another is reading/writing.

The iterateConnections() approach has the advantage that, although any one user of it could cause problems, if all its users are individually well-behaved (i.e. short-running) then there won't be any problems from interactions.  (I can explain this clearly in comments on the function declarations.)  The strong reference approach doesn't have that guarantee -- all users might be well-behaved but their interactions could cause havoc.

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to Nicholas Nethercote [:njn] from comment #16)
> I don't think it's safe;  it's horribly racy.  The strong references ensure
> that no Connection is destroyed while multiple threads are doing their
> thing, but you could have multiple threads reading and writing Connections
> at the same time, and you could also have one thread destroy the underlying
> SQLite connection while another is reading/writing.
Either I'm misunderstanding what you are writing here, or it's wrong.  `mRegistrationMutex` would protect against multiple threads reading/writing to our list.  However, strong references don't work because we unregister in the destructor, and you can't `AddRef` then.

> The iterateConnections() approach has the advantage that, although any one
> user of it could cause problems, if all its users are individually
> well-behaved (i.e. short-running) then there won't be any problems from
> interactions.  (I can explain this clearly in comments on the function
> declarations.)  The strong reference approach doesn't have that guarantee --
> all users might be well-behaved but their interactions could cause havoc.
The thing that I am most worried about is code that uses this that ends up calling outside of the module.  That's a big no-no when holding a lock (we already do it in some places because the SQLite API leaves us with no choice, sadly).

Taking a step back, you want to track open connections.  Right now we track both open and "closed but not yet deleted" connections with the given patch.  However, unregistering in `Close` doesn't buy us anything because connections are not always closed.  What if we just store strong references in `Service`?


(I think we can all agree that threads with shared memory suck at this point :/)

Comment 18

[Nicholas Nethercote [inactive]]

> Taking a step back, you want to track open connections.  Right now we track
> both open and "closed but not yet deleted" connections with the given patch.
> However, unregistering in `Close` doesn't buy us anything because
> connections are not always closed.  What if we just store strong references
> in `Service`?

I must be misunderstanding your strong references proposal.  Can you sketch out the proposed API, including the type of |mConnections|?

Comment 19

[Shawn Wilsher :sdwilsh]

We still keep the register and unregister like we have now.  We call register in the same place, and unregister in `Release` only when our refcount is exactly one (the one reference will be held by `mConnections`).  Alternatively, on `Connection`, track if we are registered and unregister (assuming proper flag) in both `Release` and `internalClose` such that we free the reference when we close the database.

`mConnections` is of type `nsTArray<nsRefPtr<Connection> >`.

This means we'll never hand out dead or dying objects (at least I'm pretty sure; spent a lot of time thinking about this last night while trying to fall asleep) because we'll never unregister without a refcount > 1, so the destructor should never be called.

Comment 20

[Nicholas Nethercote [inactive]]

And what's the API for the iteration?

Comment 21

[Shawn Wilsher :sdwilsh]

(In reply to Nicholas Nethercote [:njn] from comment #20)
> And what's the API for the iteration?
You can just hand out a copy of the `nsTArray`.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This proposal will mean that you have a safe list of strong refs to iterate over, but it does nothing to actually pause the other threads that may be using those connections. How would you reliably gather memory information from a connection that is currently doing database stuff on another thread?

Comment 23

[Nicholas Nethercote [inactive]]

Attachment: patch v3

(In reply to Shawn Wilsher :sdwilsh from comment #19)
> We still keep the register and unregister like we have now.  We call
> register in the same place, and unregister in `Release` only when our
> refcount is exactly one (the one reference will be held by `mConnections`). 
> 
> `mConnections` is of type `nsTArray<nsRefPtr<Connection> >`.
> 
> > And what's the API for the iteration?
> You can just hand out a copy of the `nsTArray`.

The attached patch is my attempt to do that.  bent helped me so much he might as well have written it himself (thanks, bent!)  It appears to work and the follow-up patch I'm about to post in bug 696498 which uses the list also works.

appendConnections() should probably have more comments warning people to be careful when iterating over the connections?


(In reply to ben turner [:bent] from comment #22)
> This proposal will mean that you have a safe list of strong refs to iterate
> over, but it does nothing to actually pause the other threads that may be
> using those connections. How would you reliably gather memory information
> from a connection that is currently doing database stuff on another thread?

That's what I was trying to explain in comment 16.  I suspect sdwilsh isn't worrying about this because each Connection (and all the possible operations on it) is protected by a mutex?

Comment 24

[Shawn Wilsher :sdwilsh]

(In reply to Nicholas Nethercote [:njn] from comment #23)
> That's what I was trying to explain in comment 16.  I suspect sdwilsh isn't
> worrying about this because each Connection (and all the possible operations
> on it) is protected by a mutex?
Correct.  This will be a bit more complicated when we implement asynchronous connection objects.

Comment 25

[Shawn Wilsher :sdwilsh]

Comment on attachment 578159 [details] [diff] [review]
patch v3

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

r=sdwilsh

::: storage/src/mozStorageConnection.cpp
@@ +499,5 @@
> +{
> +  NS_PRECONDITION(0 != mRefCnt, "dup release");
> +  nsrefcnt count = NS_AtomicDecrementRefcnt(mRefCnt);
> +  NS_LOG_RELEASE(this, count, "Connection");
> +  if (1 == count) {

note: I've been told in the past that this style is not wanted in the code base.  I prefer it, but don't care either way.  Just wanted to give you a heads up.

@@ +500,5 @@
> +  NS_PRECONDITION(0 != mRefCnt, "dup release");
> +  nsrefcnt count = NS_AtomicDecrementRefcnt(mRefCnt);
> +  NS_LOG_RELEASE(this, count, "Connection");
> +  if (1 == count) {
> +    // If the refcount is 1, the single reference must be from mConnections.

`mConnections` where?  Also include the class name please.

::: storage/src/mozStorageService.cpp
@@ +326,5 @@
> +  nsRefPtr<Service> kungFuDeathGrip(this);
> +  {
> +    mRegistrationMutex.AssertNotCurrentThreadOwns();
> +    MutexAutoLock mutex(mRegistrationMutex);
> +    bool removed = mConnections.RemoveElement(aConnection);

Please use `DebugOnly` here so we don't warn about an unused variable.

@@ +337,5 @@
> +Service::appendConnections(nsTArray<nsRefPtr<Connection> >& aConnections)
> +{
> +  mRegistrationMutex.AssertNotCurrentThreadOwns();
> +  MutexAutoLock mutex(mRegistrationMutex);
> +  aConnections.AppendElements(mConnections);

Be sure to `Clear` `aConnections` first.

::: storage/src/mozStorageService.h
@@ +135,5 @@
> +   *         The list of connections that is appended to.  Usually this will be
> +   *         empty.
> +   * @return The open connections.
> +   */
> +  void appendConnections(nsTArray<nsRefPtr<Connection> >& aConnections);

This wasn't obvious to me at all at first that it was an inout param.  Why not just return `nsTArray<nsRefPtr<Connect> >`?  I realize this probably makes two copies, but this is only for about:memory, and it's not terribly expensive to do so.

If anything, I'd rather have the method called `getConnections` and it should clear the `nsTArray` if we continue to pass it in.

@@ +156,5 @@
> +   */
> +  Mutex mRegistrationMutex;
> +
> +  /**
> +   * The list of connections we have created.

I tend to prefer if we also document that this variable is protected by the mutex.  That way, if they ever get separated, it's more obvious.

Comment 26

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14a57310fe58

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/14a57310fe58