logger thread should be joined on exit

RESOLVED FIXED in 3.12

Status

NSS
Tools
P3
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Julien Pierre, Assigned: Julien Pierre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.56 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
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.
(Assignee)

Updated

10 years ago
Priority: -- → P3
Target Milestone: --- → 3.12
(Assignee)

Comment 1

10 years ago
Created attachment 305907 [details] [diff] [review]
join logger thread, if started
Attachment #305907 - Flags: review?(nelson)
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);
                           ^^
Attachment #305907 - Flags: review?(nelson) → review-
(Assignee)

Comment 3

10 years ago
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.
(Assignee)

Comment 4

10 years ago
Created attachment 306149 [details] [diff] [review]
also make thread joinable
Attachment #305907 - Attachment is obsolete: true
Attachment #306149 - Flags: review?(nelson)
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?
(Assignee)

Comment 6

10 years ago
Nelson,

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 on attachment 306149 [details] [diff] [review]
also make thread joinable

r=nelson
Attachment #306149 - Flags: review?(nelson) → review+
(Assignee)

Comment 8

10 years ago
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
done
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.