Open
Bug 1027818
Opened 10 years ago
Updated 2 years ago
provide PR_ASSERT_CURRENT_THREAD_NOT_IN_MONITOR
Categories
(NSPR :: NSPR, defect)
NSPR
NSPR
Tracking
(Not tracked)
REOPENED
People
(Reporter: bkelly, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
9.73 KB,
patch
|
Details | Diff | Splinter Review |
So, in bug 1027772 I'd like to implement ReentrantMonitor::AssertNotCurrentThreadIn() in gecko. It seems, however, this class is just a wrapper around nspr. So in order to implement this I really need a macro like PR_ASSERT_CURRENT_THREAD_IN_MONITOR() for the !locked case. I see this was not implemented in bug 476536 at the time. Is it now possible?
Comment 1•10 years ago
|
||
I think we can't assert the current thread is not in the monitor or doesn't own the lock. (This is why such functions/macros weren't added in bug 476536.) The reason is that to check the condition requires entering the monitor or acquiring the lock. So this bug should be closed WONTFIX.
Reporter | ||
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Comment 2•10 years ago
|
||
We should remove the API then. I was once confused, it can be freely added to the code and silently pass the build and actually do nothing. So it's a foot-gun actually.
Comment 3•10 years ago
|
||
Honza: I agree that ReentrantMonitor::AssertNotCurrentThreadIn should be removed, and replaced with a comment that explains why it's not available. The discussion of the difficulty of implementing this function started in bug 476536 comment 8 and ended in bug 476536 comment 19. I just took a quick look to refresh my memory. It seems that it is possible but expensive to implement this function, whereas the ReentrantMonitor::AssertCurrentThreadIn function can be easily implemented.
Comment 4•10 years ago
|
||
(In reply to Wan-Teh Chang from comment #1) > I think we can't assert the current thread is not in the monitor or doesn't > own > the lock. (This is why such functions/macros weren't added in bug 476536.) I just noticed this bug recently. After reading bug 476536, I still don't understand why AssertNotCurrentThreadIn is hard to implement. It appears to me that |PR_GetMonitorEntryCount() > 0 iff current thread is in the monitor| and |PR_GetMonitorEntryCount() == 0 iff current thread is not in the monitor| should hold.
Flags: needinfo?(wtc)
Comment 5•10 years ago
|
||
JW: it is hard to implement the function efficiently. To assert the current thread isn't in the monitor, we need to either - acquire the internal lock of the monitor, check that the current thread is not the owner of the monitor, and release the internal lock; or - maintain a list of monitors owned by the current thread. The first approach is essentially what you proposed. I found that my checkin https://hg.mozilla.org/projects/nspr/rev/b15b05fafb4f already implemented this. So we can fix this bug now and update the warning comment in prmon.h. On the other hand, the similar warning comment in prlock.h still holds: we can only provide PR_ASSERT_CURRENT_THREAD_OWNS_LOCK but not PR_ASSERT_CURRENT_THREAD_DOES_NOT_OWN_LOCK.
Flags: needinfo?(wtc)
Comment 6•10 years ago
|
||
Agreed. PR_AssertCurrentThreadNotOwnsLock() is surely hard to implement. However, PR_AssertCurrentThreadNotInMonitor() is all we need now and it should benefit most of our cases. Can we reopen this bug for implementation now?
Flags: needinfo?(wtc)
Comment 7•10 years ago
|
||
JW: yes, you can start implementing this.
Status: RESOLVED → REOPENED
Flags: needinfo?(wtc)
Resolution: WONTFIX → ---
Comment 8•10 years ago
|
||
I forgot to say: I think it is unlikely that we will switch to a PRMonitor implementation that will make it hard to implement PR_ASSERT_CURRENT_THREAD_NOT_IN_MONITOR efficiently. The old (thread-unsafe) PRMonitor implementation had this property.
Comment 9•10 years ago
|
||
(In reply to Wan-Teh Chang from comment #8) > I forgot to say: I think it is unlikely that we will switch > to a PRMonitor implementation that will make it hard to > implement PR_ASSERT_CURRENT_THREAD_NOT_IN_MONITOR efficiently. > The old (thread-unsafe) PRMonitor implementation had this > property. Sorry, I don't understand your point. Do you mean current implementation of our PRMonitor allows us to implement PR_ASSERT_CURRENT_THREAD_NOT_IN_MONITOR efficiently? So we should have no concern about implementing PR_ASSERT_CURRENT_THREAD_NOT_IN_MONITOR.
Comment 10•10 years ago
|
||
As indicated in bug 476536 comment 1, it looks like we can now implement PR_AssertCurrentThreadInMonitor() as PR_ASSERT(PR_InMonitor(mon)) and PR_AssertCurrentThreadNotInMonitor() as PR_ASSERT(!PR_InMonitor(mon)). Furthermore, I think we can also export PR_InMonitor so that the client code can choose its own assertion scheme other than PR_ASSERT. For example, ReentrantMonitor::AssertCurrentThreadIn() can be implemented as MOZ_ASSERT(PR_InMonitor(mReentrantMonitor)) or NS_ASSERTION(PR_InMonitor(mReentrantMonitor)). Hi Nelson, Can you give some comments about this approach?
Flags: needinfo?(nelson)
Comment 12•10 years ago
|
||
(In reply to JW Wang [:jwwang] from comment #9) > (In reply to Wan-Teh Chang from comment #8) > > I forgot to say: I think it is unlikely that we will switch > > to a PRMonitor implementation that will make it hard to > > implement PR_ASSERT_CURRENT_THREAD_NOT_IN_MONITOR efficiently. > > The old (thread-unsafe) PRMonitor implementation had this > > property. > > Sorry, I don't understand your point. Do you mean current implementation of > our PRMonitor allows us to implement PR_ASSERT_CURRENT_THREAD_NOT_IN_MONITOR > efficiently? So we should have no concern about implementing > PR_ASSERT_CURRENT_THREAD_NOT_IN_MONITOR. That should be indeed what he meant. (In reply to JW Wang [:jwwang] from comment #10) > Furthermore, I think we can also export PR_InMonitor so that the client code > can choose its own assertion scheme other than PR_ASSERT. For example, > ReentrantMonitor::AssertCurrentThreadIn() can be implemented as > MOZ_ASSERT(PR_InMonitor(mReentrantMonitor)) or > NS_ASSERTION(PR_InMonitor(mReentrantMonitor)). I'm not an NSPR peer, so I can't comment anything terribly correct here. But I think this is rather less beneficial than might appear, because PR_InMonitor itself needs to use PR_ASSERT internally.
Flags: needinfo?(Pidgeot18)
Comment 14•10 years ago
|
||
implement PR_ASSERT_CURRENT_THREAD_IN_MONITOR and PR_ASSERT_CURRENT_THREAD_NOT_IN_MONITOR using PR_InMonitor().
Attachment #8510027 -
Flags: review?(wtc)
Updated•9 years ago
|
Flags: needinfo?(nelson)
Comment 15•2 years ago
|
||
Clear a needinfo that is pending on an inactive user.
For more information, please visit auto_nag documentation.
Flags: needinfo?(wtc)
Comment 16•2 years ago
|
||
The bug assignee is inactive on Bugzilla, so the assignee is being reset.
Assignee: wtc → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•