[webkit-dev] WTF::currentThread() and non-WTF threads

[Jeremy Orlow]

On Fri, Dec 11, 2009 at 4:08 PM, Dmitry Titov <dimich at chromium.org> wrote:

> Thanks for sharing your thoughts!
>
> On Fri, Dec 11, 2009 at 2:44 AM, Jeremy Orlow <jorlow at chromium.org> wrote:
>
>> On Thu, Dec 10, 2009 at 6:48 PM, Dmitry Titov <dimich at chromium.org>wrote:
>>
>>> Hi webkit-dev!
>>>
>>> Trying to add ASSERTS to RefCounted objects (
>>> https://bugs.webkit.org/show_bug.cgi?id=31639) I've added some more
>>> calls to WTF::currentThread() and started to get a lot of assertions here:
>>>
>>> static ThreadIdentifier establishIdentifierForPthreadHandle(pthread_t&
>>> pthreadHandle)
>>> {
>>>     ASSERT(!identifierByPthreadHandle(pthreadHandle));
>>>
>>> Here is what happens: A thread that is not created by WTF::CreateThread
>>> is calling currentThread() in hope to get a ThreadIdentifier. It doesn't
>>> have it so we create it and put into ThreadMap. Normally, WTF threads call
>>> WTF::detachThread() that removes the ThreadIdentifier from the map. The
>>> 'other' threads do not do so - so the TheradIdentifier remains in the map.
>>>
>>> On OSX, pthreads reuse system handles when one thread terminates and
>>> another starts... That easily leads to threads that run sequentially to have
>>> the same handle, and the same ThreadIdentifier - since it is still in the
>>> threadMap, which makes currentThread() kind of useless since it returns the
>>> same id for different threads, the very thing the threadMap is tryign to
>>> avoid.
>>>
>>> I'm thinking about changing the implementation to stop "auto-register"
>>> non-WTF threads.
>>>
>>
>> Wouldn't this break binary compatibility?
>>
>
> If you mean nightly builds, OSX Safari runs fine with this. On Windows it's
> going to be ok because we use OS-supplied ThreadIdentifiers there.
>

WebKit is a framework on mac....so I was wondering if any app that depends
on the framework will work OK.  I have no idea, but my guess is this would
be an issue.


>  Does pthreads have any facility to register a callback that's called on
>> thread shutdown?  Could we use some sort of thread local storage to keep
>> track of whether that thread has been auto-registered yet?
>>
>
> I think I see what you mean. I tried to figure out if it is possible. There
> is a callback invoked on destruction of a thread-specific slot... Perhaps it
> can be used to un-register the thread if we add a thread-specific slot to
> all incoming threads. The problem with those is that they are called in
> unspecified order so another thread-specific destructor could re-register a
> thread again, by calling currentThread(). Perhaps there is a way to do this
> cleanly, it escapes me at the moment...
>

What if the thread local storage kept track of the threads identifier?  If
it's empty, then we need to auto-register.  If it's there, then we use that.
 I think that would avoid the race you mentioned.


> Maybe all non-registered threads could just use a default thread identifier
>> and some sort of warning message could be logged the first time this
>> happens?
>>
>
> Most usages of currentThread() are like this:
> ASSERT(m_myThreadId == currentThread());
> Basically, they verify that the object is called on the same thread as
> created... Returning same identifier for different threads satisfies this
> check even if the treads are different. If currentThread() returned the
> different identifier each time in case of un-registered thread, it would
> force the developer to either use WTF::createThread or WTF::registerThread.
>

If all non-WTF threads used the same identifier, then all blah ==
currentThread() asserts would work.  Some of the less common blah !=
currentThread() asserts could break if the current thread and the one it's
comparing to are both non-WTF.  We could probably find ways around all of
those asserts.

 Instead, lets add a new function, WTF::registerThread() that would
>>> establish the ThreadIdentifier for a thread not created by WTF::createThread
>>> - and assert in currentThread() if the current thread is unknown to WTF.
>>> This way, we could find all the cases of such threads and equip them with
>>> registerThread()/detachThread() pair that will keep the thread map in a good
>>> state.
>>>
>>> The currentThread() would look like this:
>>>
>>> ThreadIdentifier currentThread()
>>> {
>>>     pthread_t currentThread = pthread_self();
>>>     if (ThreadIdentifier id = identifierByPthreadHandle(currentThread))
>>>         return id;
>>>
>>>     ASSERT_NOT_REACHED();
>>>
>>>     // Either the thread is not created by WTF::CreateThread() or
>>> registered by WTF::registerThread(), or we are getting
>>>     // a call from thread-specific destructor after WTF::detachThread()
>>> was called and ThreadIdentifier removed.
>>>     // Neither scenario permits reliable thread id tracking, so we can
>>> not return a meaningful ThreadIdentifier here.
>>>     // Normally ThreadIdentifiers are compared so lets generate a fake
>>> one-time ThreadIdentifier to fail comparison, if
>>>     // it is in fact done.
>>>     static ThreadIdentifier fakeId = minFakeIdentifierCount;
>>>
>>>     if (fakeId == maxFakeIdentifierCount)
>>>         fakeId = minFakeIdentifierCount;
>>>
>>>     return fakeId++;
>>> }
>>>
>>> What would you say? Any bad feelings about that?
>>>
>>> Dmitry
>>>
>>> _______________________________________________
>>> webkit-dev mailing list
>>> webkit-dev at lists.webkit.org
>>> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20091211/ff243219/attachment.html>