Open Bug 1027818 Opened 10 years ago Updated 2 years ago

provide PR_ASSERT_CURRENT_THREAD_NOT_IN_MONITOR

Categories

(NSPR :: NSPR, defect)

defect

Tracking

(Not tracked)

REOPENED

People

(Reporter: bkelly, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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?
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
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.
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.
(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)
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)
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)
JW: yes, you can start implementing this.
Status: RESOLVED → REOPENED
Flags: needinfo?(wtc)
Resolution: WONTFIX → ---
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.
(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.
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)
Hi Joshua,
Can you comment comment 10? Thanks.
Flags: needinfo?(Pidgeot18)
(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)
Hi Wan-Teh,
Can you comment comment 10? Thanks.
Flags: needinfo?(wtc)
implement PR_ASSERT_CURRENT_THREAD_IN_MONITOR and PR_ASSERT_CURRENT_THREAD_NOT_IN_MONITOR using PR_InMonitor().
Attachment #8510027 - Flags: review?(wtc)
Flags: needinfo?(nelson)

Clear a needinfo that is pending on an inactive user.

For more information, please visit auto_nag documentation.

Flags: needinfo?(wtc)

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: wtc → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: