Closed Bug 232958 Opened 20 years ago Closed 20 years ago

[BEOS] Condvar implementation is not properly implemented

Categories

(NSPR :: NSPR, defect)

x86
BeOS
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: thesuckiestemail, Assigned: wtc)

References

Details

(Keywords: fixed1.7)

Attachments

(2 files, 7 obsolete files)

User-Agent:       
Build Identifier: 

When quitting mozilla for instance several threads doesn't get notified. This
causes them to hang until timeout occurs making quit take up about two minutes
in worst case.

I

Reproducible: Always
Steps to Reproduce:
1. Quit Mozilla


Actual Results:  
Mozilla application lives without windows for a long time before shutting down.

Expected Results:  
Mozilla should quit almost instantly.

I have a incomplete implementation that does not have this behaviour. It passes
the cvar and cvar2 tests though.
Attached patch Patch for prImpl.h (obsolete) — Splinter Review
This is work in progress. Just submitting so others can test it. Needs New
btvar.c too.
Attached file New btcvar.c (incomplete) (obsolete) —
Not sure how to deal with PR_INTERVAL_NO_TIMEOUT and PR_INTERVAL_NO_WAIT yet.
Otherwise ok. We could probably us benaphores to get more lightweight condvar
in final patch.

Submitting this as I will be away for a few days.
Adding CC
Also i think that numerous bugs about quitting problems should be set dependent
on this. If not DUP
*** Bug 225814 has been marked as a duplicate of this bug. ***
Decaf, when you 'll be back, can you look if there is reason to use benaphores
also in widget code (nsAppShell, nsToolkit) instead semaphores?
Attached patch Updated patch for primpl.h (obsolete) — Splinter Review
Attachment #140500 - Attachment is obsolete: true
Attached patch Updated btcvar.c (obsolete) — Splinter Review
Not sure if PR_INTERVAL_NO_TIMEOUT and PR_INTERVAL_NO_WAIT is implemented
correctly. Plus there might be some issues with benaphore locking.
Attachment #140501 - Attachment is obsolete: true
Attachment #141767 - Attachment description: Updated to use a benaphore (lighter semaphore) for internal synchronization. → Updated patch for primpl.h
Attachment #141768 - Attachment description: Updated to use a benaphore (lighter semaphore) for internal synchronization. → Updated btcvar.c
Comment on attachment 141767 [details] [diff] [review]
Updated patch for primpl.h

Requesting review
Attachment #141767 - Flags: review?
Attachment #141768 - Flags: review?
Fredrik, you should probably set the review flag to a specific reviewer...

Prog.
(In reply to comment #10)
> Fredrik, you should probably set the review flag to a specific reviewer...
> 
> Prog.

Well, I don't know anyone that are good for this kind of review. I would want
someone who knows how cvar is supposed to work with flags and that has a keen
eye for possible race conditions.
Fredrik, why btcvar is submitted in form of file, not patch?
I doubt that it is acceptable.
I'm trying to find someone for real internal review.
As far as i understand, first formal review may be from me or Simon, if and when
we receive opinion of knowleable person, but then it needs superreview, probably
by wchang. As far as i remember, he understood BeOS specifics quite well, not to
say about being nspr God.
(In reply to comment #12)
> Fredrik, why btcvar is submitted in form of file, not patch?
> I doubt that it is acceptable.

It's completly rewritten, it has nothing in common with the old code so it would
first remove every line and then add every new line. This is shorter.
Cooment from OBEOS kernel developer Axel Doerfler:
"I had a second look, and despite the timeout thing, I think the rest 
could actually work - at least it follows Be's sample code quite 
closely. While I am not convinced that this is the best solution to 
this problem, and it's also done quite complicated (factoring out the 
benaphore mechanism would make the code already much simpler), I think 
you should switch to this solution.
The original version is completely broken.

Bye,
   Axel."
---------------
Timeout thing:
"One minor problem of the new one is that it takes the "timeout" 
variable directly without bothering to use the 
PR_IntervalToMicroseconds(), although I am not sure if it's needed (but 
it would be definitely cleaner to use it)."
(I concur, I think it could be done much better if I had found some better
documentation on implementing condvars. I even looked thru my old Nachos docs.)

I will fix the timeout. I assumed it was microseconds as it wasn't documented.
Stupid me. I will update btcvar.c and set Sergei as review as Axel's words and
Sergeis review should be sufficient. 
Added timeout conversion and submitted as 'cvs diff -uwp' patch.
Attachment #141768 - Attachment is obsolete: true
Comment on attachment 141767 [details] [diff] [review]
Updated patch for primpl.h

review request
Attachment #141767 - Flags: review? → review?(sergei_d)
Attachment #143430 - Flags: review?(sergei_d)
Attachment #141768 - Flags: review?
will test it today/tomorrow.
Is this btcvar diff over existing cvs implementation or over new one from this page?
About http://bugzilla.mozilla.org/attachment.cgi?id=141767&action=view
what does it mean? 
are these outcommented line intentionally here?
+//    sem_id	isem;		/* Semaphore used to lock threadQ */
+//    int32	benaphoreCount; /* Number of people in lock */
(In reply to comment #20)
> About http://bugzilla.mozilla.org/attachment.cgi?id=141767&action=view
> what does it mean? 
> are these outcommented line intentionally here?
> +//    sem_id	isem;		/* Semaphore used to lock threadQ */
> +//    int32	benaphoreCount; /* Number of people in lock */

Oops, they should be removed. As you can see that is part of the old implementation.
(In reply to comment #19)
> will test it today/tomorrow.
> Is this btcvar diff over existing cvs implementation or over new one from this
page?

It's a CVS diff. ( cvs diff -uwp btcvar.c )
Comment on attachment 141767 [details] [diff] [review]
Updated patch for primpl.h

r=sergei_d@fi.tartu.ee

Remove mentioned outcommented code before asking wtc about review and checkin
Attachment #141767 - Flags: review?(sergei_d) → review+
Comment on attachment 143430 [details] [diff] [review]
btcvar.c in patchform and with timeout conversion

r=sergei_d@fi.tartu.ee

It works. Though, there is outcommented old code in this patch too - see
notices for primpl.h review.
Attachment #143430 - Flags: review?(sergei_d) → review+
Attached patch Cleaned up primpl.h (obsolete) — Splinter Review
Cleaned up primpl.h according to comments. Same functionality.
Attachment #141767 - Attachment is obsolete: true
Attached patch Cleaned up btcvar.c (obsolete) — Splinter Review
Cleaned up btcvcar.c according to comments. Same functionality.
Attachment #143430 - Attachment is obsolete: true
Comment on attachment 144337 [details] [diff] [review]
Cleaned up primpl.h

Need sr to this important patch. (And someone to commit on success).
Attachment #144337 - Flags: superreview?(wchang0222)
Comment on attachment 144338 [details] [diff] [review]
Cleaned up btcvar.c

See previous comment.
Attachment #144338 - Flags: superreview?(wchang0222)
targeting Seamonkey 1.7 milestone
Comment on attachment 144337 [details] [diff] [review]
Cleaned up primpl.h

r=wtc.
Attachment #144337 - Flags: superreview?(wchang0222) → superreview+
Comment on attachment 144337 [details] [diff] [review]
Cleaned up primpl.h

Would be nice to add comments that explain each field
(like the original code did).
Comment on attachment 144338 [details] [diff] [review]
Cleaned up btcvar.c

r=wtc.

Two comments.

1. I did not review the algorithm.  I only made
sure that all the changes are in BeOS-specific
code.

It is a hard problem to implement condition
variables using semaphores.  The pthreads-win32
library had to solve this problem, and they
went through several algorithms before getting
it right.

2. For me to check in these two patches, I need
them regenerated without the -w flag.
Attachment #144338 - Flags: superreview?(wchang0222) → superreview+
Comment on attachment 144338 [details] [diff] [review]
Cleaned up btcvar.c

Would be nice to use a consistent style for the placement
of curly braces { }.
Attached patch Final primpl.h?Splinter Review
Attachment #144337 - Attachment is obsolete: true
Attached patch final btcvar.c?Splinter Review
New patches according to notes.

(Your comments on other impl. noted.)
Attachment #144338 - Attachment is obsolete: true
Attachment #145031 - Flags: review?
Attachment #145032 - Flags: review+
Attachment #145031 - Flags: review? → review+
Wan-Teh Chang, could you check in the latest patches and close bug?
Comment on attachment 145031 [details] [diff] [review]
Final primpl.h?

Requesting mozilla 1.7 approval.  This is a BeOS-only change.
Attachment #145031 - Flags: approval1.7?
Comment on attachment 145032 [details] [diff] [review]
final btcvar.c?

Requesting mozilla 1.7 approval.  This is a BeOS-only change.
Attachment #145032 - Flags: approval1.7?
Comment on attachment 145032 [details] [diff] [review]
final btcvar.c?

a=dveditz for 1.7
Attachment #145032 - Flags: approval1.7? → approval1.7+
Comment on attachment 145031 [details] [diff] [review]
Final primpl.h?

a=dveditz for 1.7
Attachment #145031 - Flags: approval1.7? → approval1.7+
I checked in the two patches on the NSPR trunk (NSPR 4.6)
and NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 alpha).
Status: NEW → ASSIGNED
Target Milestone: --- → 4.5
I checked in the two patches on the NSPR_4_5_BRANCH (NSPR 4.5)
and MOZILLA_1_7_BRANCH (Mozilla 1.7 final).
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Thank you!
Keywords: fixed1.7
You need to log in before you can comment on or make changes to this bug.