328790 - main thread never joins with async-io thread

Status: RESOLVED FIXED

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

Description

[Darin Fisher]

main thread never joins with async-io thread

the wait(cvar) code in FinishAsyncIO should use PR_JoinThread (or nsIThread::join) instead.  otherwise, kernel thread resources are never released.

Comment 1

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

Attachment: AsyncWrite thread cleanup trunk patch v1.0

I've been doing a lot of thread work recently and figured I would submit a patch on this one... partly to help out and partly to make sure that I'm doing my own things correctly ;-)

BTW, if you try Join() on a thread created without the PR_JOINABLE_THREAD flag set then the app dies quietly. Does that mean that unjoinable threads will always exhibit this leak you're talking about?

Comment 2

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

Attachment: A -w version of the patch [NOT FOR CHECKIN]

For easier reading

Comment 3

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

Attachment: AsyncWrite thread cleanup trunk patch v1.1

After thinking about this for a second I realized that we no longer need AsyncThreadedMode as we can just test the value of AsyncWriteThreadInstance instead. This patch takes care of that.

Comment 4

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

Attachment: A -w version of the v1.1 patch [NOT FOR CHECKIN]

Again for easier reading

Comment 5

[Brett Wilson]

Comment on attachment 215545 [details] [diff] [review]
AsyncWrite thread cleanup trunk patch v1.1

>-PRBool AsyncThreadedMode = PR_FALSE;
>+static nsCOMPtr<nsIThread> AsyncWriteThreadInstance = nsnull;

This should probably be a raw pointer instead of a COM pointer. The COM pointer doesn't really get you anything because it will hold it for the life of the program. Also trying to free the COM object on shutdown might get weird (to paraphrase Darin).

+  // release the thread and enter single-threaded mode
+  AsyncWriteThreadInstance = nsnull;

Then add a NS_RELEASE before here.

r=brettw with that fixed.

Comment 6

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

Attachment: AsyncWrite thread cleanup trunk patch v1.2

With those comments.

Comment 7

[Brett Wilson]

Comment on attachment 215726 [details] [diff] [review]
AsyncWrite thread cleanup trunk patch v1.2


>+  nsresult rv = NS_NewThread(&AsyncWriteThreadInstance,
>+                             thread,
>+                             0,
>+                             PR_JOINABLE_THREAD);
>+  NS_ADDREF(AsyncWriteThreadInstance);
>   if (NS_FAILED(rv)) {
>-    AsyncThreadedMode = PR_FALSE;
>+    NS_RELEASE(AsyncWriteThreadInstance);

AsyncWriteThreadInstance should be NULL if the call fails. Calling release on it will crash. You can just take this NS_RELEASE, I am not worried about leaking an object that shouldn't exist (setting to NULL below seems good just to be safe). If you *really* want, you can do NS_IF_RELEASE.

r=me with that fixed.

Comment 8

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

Attachment: AsyncWrite thread cleanup trunk patch v1.3

Yow, I totally spaced out on that one. This should do it.

Comment 9

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

fixed on trunk

Comment 10

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

Comment on attachment 215729 [details] [diff] [review]
AsyncWrite thread cleanup trunk patch v1.3

should we put this on the branch?

Comment 11

[Darin Fisher]

Comment on attachment 215729 [details] [diff] [review]
AsyncWrite thread cleanup trunk patch v1.3

yes, i think it makes sense to keep mozilla/storage/ synchronized between trunk and MOZILLA_1_8_BRANCH

Comment 12

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

fixed on branch

Comment 13

[shutdown]

(In reply to comment #8)
> Created an attachment (id=215729) [edit]
> AsyncWrite thread cleanup trunk patch v1.3
> Yow, I totally spaced out on that one. This should do it.

|+  nsresult rv = NS_NewThread(&AsyncWriteThreadInstance,
|+                             thread,
|+                             0,
|+                             PR_JOINABLE_THREAD);
|   if (NS_FAILED(rv)) {
|-    AsyncThreadedMode = PR_FALSE;
|+    AsyncWriteThreadInstance = nsnull;
|     return rv;
|   }
|+  NS_ADDREF(AsyncWriteThreadInstance);

NS_NewThread() returns already AddRef'ed nsIThread*, so NS_ADDREF()ing
AsyncWriteThreadInstance increments its refcnt to 2. As a result,
NS_RELEASE(AsyncWriteThreadInstance) in FinishAsyncIO() never
decrements nsThread instance's refcnt to 0, and it leaks.

Comment 14

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

Attachment: Doh - remove that pesky AddRef

Uh, oops. Indeed.

Brett and I talked about this... Not sure how it made it into the final checkin.

Comment 15

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

Comment on attachment 217537 [details] [diff] [review]
Doh - remove that pesky AddRef

Fixed on trunk and branch.

Thanks for the heads-up, shutdown.