If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

provide PR_ASSERT_CURRENT_THREAD_NOT_IN_MONITOR

REOPENED
Assigned to

Status

NSPR
NSPR
REOPENED
3 years ago
2 years ago

People

(Reporter: bkelly, Assigned: Wan-Teh Chang, NeedInfo)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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?
(Assignee)

Comment 1

3 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

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 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.
(Assignee)

Comment 3

3 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.
(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)
(Assignee)

Comment 5

3 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)
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)
(Assignee)

Comment 7

3 years ago
JW: yes, you can start implementing this.
Status: RESOLVED → REOPENED
Flags: needinfo?(wtc)
Resolution: WONTFIX → ---
(Assignee)

Comment 8

3 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.
(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)
Created attachment 8510027 [details] [diff] [review]
1027818_implement_PR_AssertCurrentThreadNotInMonitor.patch

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)
You need to log in before you can comment on or make changes to this bug.