Closed Bug 487344 Opened 13 years ago Closed 13 years ago

Need an abstract class that implements thread-safe nsISupports methods

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: mozilla+ben, Assigned: mozilla+ben)

Details

Attachments

(1 file, 2 obsolete files)

Implementing thread-safe reference counting is not quite as easy as it seems.  The NS_IMPL_THREADSAFE_* macros ensure atomicity, but Release calls still need to be proxied back to the main thread.
Templatized to permit arbitrary interfaces.
Simpler implementation for the Release override, consequently.
Attachment #371580 - Attachment is obsolete: true
Following a conversation with Ben T.  See the comments for further motivation.
Attachment #371582 - Attachment is obsolete: true
Attachment #371631 - Flags: review?(bent.mozilla)
I mostly don't understand the motivation for this patch: what particular semi-threadsafe classes would this be used for? The talk of cycle collection in particular makes no sense, because the cycle collector cannot deal with objects that ever live off the main thread.

If this is being used for objects which are held via XPCOM proxies, I think we can/should just have a templated holder class which avoids the off-main-thread refcounting.
(In reply to comment #4)
> I mostly don't understand the motivation for this patch: what particular
> semi-threadsafe classes would this be used for?

The misunderstanding may be entirely mine, but my intention was to have functors (more precisely, FunctorImpls; see bug 486440) inherit from MainThreadReleasedSupports<nsISupports>.  That way, if a functor had any bound arguments that needed to be released on the main thread, they would be.  Another class, called nsRunnableFunctor in the current functor patch, could inherit from MainThreadReleasedSupports<nsIRunnable>.  Whether or not the NS_ProxyRelease is worthwhile, this patch would unify some duplicated code in my functor implementation, and might be useful elsewhere (?).

I definitely appreciate the SEMI-threadsafety of these classes, and I'm not trying to promise safety for all derived classes.  That's the reason I switched the name from ThreadsafeSupports (which seemed over-promising) to MainThreadReleasedSupports.

The question remains whether overriding Release helps at all, though.  I assumed it must, else what purpose would NS_ProxyRelease serve?  But now I'm not so sure.  The NS_IMPL_THREADSAFE_* versions of the nsISupports methods are definitely essential for XPCOM objects that may be shared across threads, but the main-thread Releasing may just be a faulty solution in search of a problem.  If it doesn't accomplish anything, I'm not committed to it.

> The talk of cycle collection in
> particular makes no sense, because the cycle collector cannot deal with objects
> that ever live off the main thread.

Ok, let me understand.  This is because cycle traversal could happen at any time from the main thread, so participating objects can't risk being touched simultaneously from another thread?  My thinking was that main-thread Releasing would at least guarantee that objects got Suspected on the main thread, but it's apparent to me now that there's nothing I can do to ensure they also get Forgotten on the main thread, and even participating in cycle collection is dangerous for off-main-thread objects, anyway (as you say).

> If this is being used for objects which are held via XPCOM proxies, I think we
> can/should just have a templated holder class which avoids the off-main-thread
> refcounting.

So just one class, without an overridden Release?
Another glaring problem with this patch is that it uses NS_IMPL_THREADSAFE_QUERY_INTERFACE0 regardless of whether or not the Interface type is nsISupports, so derived objects can only be QIed to nsISupports.  Switching to _QUERY_INTERFACE1(MTRS<Interface>, Interface) might work, but that's really chancy and still not flexible enough.

Unless I'm missing an easy fix, this patch no longer provides the convenience I had hoped.  Marking invalid...
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
No longer blocks: 486440
Attachment #371631 - Flags: review?(bent.mozilla)
You need to log in before you can comment on or make changes to this bug.