Closed
Bug 328790
Opened 19 years ago
Closed 18 years ago
main thread never joins with async-io thread
Categories
(Toolkit :: Storage, defect, P3)
Toolkit
Storage
Tracking
()
RESOLVED
FIXED
People
(Reporter: darin.moz, Assigned: bent.mozilla)
Details
(Keywords: fixed1.8.1)
Attachments
(2 files, 5 obsolete files)
7.99 KB,
patch
|
darin.moz
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
534 bytes,
patch
|
brettw
:
first-review+
darin.moz
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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.
Updated•18 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•18 years ago
|
||
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?
Attachment #215539 -
Flags: first-review?(darin)
Assignee | ||
Comment 2•18 years ago
|
||
For easier reading
Assignee | ||
Comment 3•18 years ago
|
||
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.
Attachment #215539 -
Attachment is obsolete: true
Attachment #215545 -
Flags: first-review?(darin)
Attachment #215539 -
Flags: first-review?(darin)
Assignee | ||
Comment 4•18 years ago
|
||
Again for easier reading
Attachment #215540 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Attachment #215545 -
Flags: second-review?(brettw)
Comment 5•18 years ago
|
||
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.
Attachment #215545 -
Flags: first-review?(darin) → first-review+
Assignee | ||
Comment 6•18 years ago
|
||
With those comments.
Attachment #215545 -
Attachment is obsolete: true
Attachment #215546 -
Attachment is obsolete: true
Attachment #215726 -
Flags: second-review?(brettw)
Attachment #215545 -
Flags: second-review?(brettw)
Comment 7•18 years ago
|
||
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.
Attachment #215726 -
Flags: second-review?(brettw) → second-review+
Assignee | ||
Comment 8•18 years ago
|
||
Yow, I totally spaced out on that one. This should do it.
Attachment #215726 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•18 years ago
|
||
Comment on attachment 215729 [details] [diff] [review] AsyncWrite thread cleanup trunk patch v1.3 should we put this on the branch?
Attachment #215729 -
Flags: approval-branch-1.8.1?(darin)
Reporter | ||
Comment 11•18 years ago
|
||
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
Attachment #215729 -
Flags: approval-branch-1.8.1?(darin) → approval-branch-1.8.1+
Comment 13•18 years ago
|
||
(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.
Assignee | ||
Comment 14•18 years ago
|
||
Uh, oops. Indeed. Brett and I talked about this... Not sure how it made it into the final checkin.
Attachment #217537 -
Flags: first-review?(brettw)
Attachment #217537 -
Flags: approval-branch-1.8.1?(darin)
Updated•18 years ago
|
Attachment #217537 -
Flags: first-review?(brettw) → first-review+
Reporter | ||
Updated•18 years ago
|
Attachment #217537 -
Flags: approval-branch-1.8.1?(darin) → approval-branch-1.8.1+
Assignee | ||
Comment 15•18 years ago
|
||
Comment on attachment 217537 [details] [diff] [review] Doh - remove that pesky AddRef Fixed on trunk and branch. Thanks for the heads-up, shutdown.
You need to log in
before you can comment on or make changes to this bug.
Description
•