Last Comment Bug 419763 - logger thread should be joined on exit
: logger thread should be joined on exit
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: trunk
: All All
: P3 normal (vote)
: 3.12
Assigned To: Julien Pierre
Depends on:
  Show dependency treegraph
Reported: 2008-02-26 18:28 PST by Julien Pierre
Modified: 2008-02-27 18:52 PST (History)
0 users
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

join logger thread, if started (2.35 KB, patch)
2008-02-26 18:34 PST, Julien Pierre
nelson: review-
Details | Diff | Splinter Review
also make thread joinable (1.56 KB, patch)
2008-02-27 15:59 PST, Julien Pierre
nelson: review+
Details | Diff | Splinter Review

Description Julien Pierre 2008-02-26 18:28:53 PST
Similar to 418360, we leave a thread running in selfserv before PR_Cleanup() is called. This can cause issues such as assertions. The fix is to start the thread as joinable and join it before PR_Cleanup.
Comment 1 Julien Pierre 2008-02-26 18:34:41 PST
Created attachment 305907 [details] [diff] [review]
join logger thread, if started
Comment 2 Nelson Bolyard (seldom reads bugmail) 2008-02-27 09:46:55 PST
Comment on attachment 305907 [details] [diff] [review]
join logger thread, if started

This patch is good, as far as it goes, but it attempts to join an 
unjoinable thread.  The thread must be made joinable when it is created.

2071    loggerThread = PR_CreateThread(PR_SYSTEM_THREAD, 
2072                    logger, NULL, PR_PRIORITY_NORMAL, 
2073                    useLocalThreads ? PR_LOCAL_THREAD:PR_GLOBAL_THREAD,
2074                    PR_UNJOINABLE_THREAD, 0);
Comment 3 Julien Pierre 2008-02-27 14:07:57 PST
Oops. Thanks for catching that, Nelson. Strangely enough, I had tested the patch (with the -L option) and didn't get a hang. It must be that PR_JoinThread skips and/or fails when trying to join unjoinable threads.
Comment 4 Julien Pierre 2008-02-27 15:59:53 PST
Created attachment 306149 [details] [diff] [review]
also make thread joinable
Comment 5 Nelson Bolyard (seldom reads bugmail) 2008-02-27 17:58:24 PST
Julien, Yes, PR_JoinThread returns PR_FAILURE if the thread is unjoinable.
Maybe your patch should detect that case and either report a warning
or report a test failure.  What do you think?
Comment 6 Julien Pierre 2008-02-27 18:15:27 PST

Yes, that would be an improvement. I think the attached patch is good enough. It seems that not a single program under cmd checks for the return value of PR_JoinThread. This being a fairly optional thread, I wouldn't think it would be the first place to check it.

I notice that we don't join the worker threads in selfserv. We wait for a thread counter to go to zero instead . This may be OK, but it's conceivable that NSPR still executes some code after the final PZ_Unlock(qLock) in thread_wrapper which could cause NSPR reinitialization, and thus conflit with PR_Cleanup as well. I'm not saying it does today, just that there is a potential race there too if we keep the PR_Cleanup call. I would prefer to see a PR_JoinThread loop in terminateWorkerThreads to what we have now.
Comment 7 Nelson Bolyard (seldom reads bugmail) 2008-02-27 18:42:59 PST
Comment on attachment 306149 [details] [diff] [review]
also make thread joinable

Comment 8 Julien Pierre 2008-02-27 18:52:41 PST
Thanks, Nelson.

I checked this in to the trunk 

Checking in selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.83; previous revision: 1.82

Note You need to log in before you can comment on or make changes to this bug.