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

[Dmitry Titov]

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.


> 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...


> 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.


>
>
>> 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/5a2ab092/attachment.html>