Bug 137195 – [WK2][EFL] Fix the problem that threads created by a DispatchQueueEfl are not destroyed even after the DispatchQueueEfl has been destructed

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Joonghun Park]

Linux and Unix System's pthread is created in joinable state by default.
So it is needed to detach the thread explicitly after dispatchQueue is destructed.
Currently, even after DispatchQueueEfl was destructed  the thread created by a DispatchQueueEfl is not destroyed 
because there is no explicit detachThread command.
It leads to maximum number of threads which is allowed by a process in platform 
in case iterating creation and deletion of plugin process many times and lock-up is occurred in conclusion.

[Joonghun Park]

Created attachment 238815 [details]
Patch

[Joonghun Park]

Created attachment 238817 [details]
Patch

[Joonghun Park]

Created attachment 238818 [details]
Patch

[Joonghun Park]

Created attachment 239390 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 239390 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239390&action=review

> Source/WebKit2/ChangeLog:3
> +        [WK2][EFL] Fix not destroyed thread created by DispatchQueueEfl even after the DispatchQueueEfl has been destructed

Not clear bug title.

> Source/WebKit2/ChangeLog:8
> +        Linux and Unix System's pthread is created in joinable state by default.

I don't get this patch description. Please improve it further.

> Source/WebKit2/ChangeLog:10
> +        The call to waitForThreadCompletion() in ~DispatchQueue() is for this purpose.

I don't read this sentence.

> Source/WebKit2/Platform/efl/DispatchQueueEfl.h:67
> +    ThreadIdentifier m_threadID;

If you declare this below bool m_isThreadRunning, you don't need to change m_isThreadRunning in DispatchQueue::DispatchQueue()

[Joonghun Park]

Created attachment 239469 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 239469 [details]
Patch

Patch looks reasonable for me. However it would be great other folks also take a look this patch. Add byungwoo to CC.

[Gyuyoung Kim]

Comment on attachment 239469 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239469&action=review

BTW I wonder whether there is any regression on EFL layout test. Did you run layout test with this patch ?

> Source/WebKit2/ChangeLog:8
> +        Linux and Unix System's pthread is created in joinable state by default.

pthread is -> pthreads are

> Source/WebKit2/ChangeLog:9
> +        It means that if the created threads are not destructed explicitly,

How about below sentence ?

"If threads aren't destructed explicitly, it will be still live until process which created the thread is dead. Besides it may cause out of capacity range of thread creation in a process. It is one of hidden defects. Thus we need to destroy the thread together when DispatchQueue's dtor is called. This patch calls waitForThreadCompletion() to destroy the thread in dtor of DispatchQueue class"

[Gyuyoung Kim]

Comment on attachment 239469 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239469&action=review

> Source/WebKit2/Platform/efl/DispatchQueueEfl.cpp:74
> +    , m_threadID(0)

One more thing. 0 -> nullptr ?

[Byungwoo Lee]

Do we have to join the created thread at the DispatchQueue destructor?
How about detaching after the thread was created? Is there any problem to do so?

[Joonghun Park]

(In reply to comment #10)
> Do we have to join the created thread at the DispatchQueue destructor?
> How about detaching after the thread was created? Is there any problem to do so?

If the dispatchQueue object is destructed prior to the entry function of the dispatchQueue thread, dispatchQueue's m_workItems is valid no more.
If under the circumstances, DispatchQueue::performWork() is called, it leads to crash or locking up. That's why we should use pthread_join in dtor of DispatchQueue   class, I think.

[Byungwoo Lee]

(In reply to comment #11)
> (In reply to comment #10)
> > Do we have to join the created thread at the DispatchQueue destructor?
> > How about detaching after the thread was created? Is there any problem to do so?
> 
> If the dispatchQueue object is destructed prior to the entry function of the dispatchQueue thread, dispatchQueue's m_workItems is valid no more.
> If under the circumstances, DispatchQueue::performWork() is called, it leads to crash or locking up. That's why we should use pthread_join in dtor of DispatchQueue   class, I think.
How can it be possible? DispatchQueue::ThreadContext has the reference of DispatchQueue and the context will be passed to the thread function.

[Byungwoo Lee]

Comment on attachment 239469 [details]
Patch

DispatchQueue can be destroyed at the ThreadContext::function because the DispatchQueue::ThreadContext has a reference of the DispatchQueue instance. Joining the thread at the DispatchQueue destructor can lead a infinite waiting of the created thread.

[Joonghun Park]

(In reply to comment #13)
> (From update of attachment 239469 [details])
> DispatchQueue can be destroyed at the ThreadContext::function because the DispatchQueue::ThreadContext has a reference of the DispatchQueue instance. Joining the thread at the DispatchQueue destructor can lead a infinite waiting of the created thread.

I examined the code carefully again, and I found that you're right.
ThreadContext object has a RefPtr and passed to ThreadContext::function() as arguments. 
So I see that the destruction sequence is garanteed what which only after ThreadContext::function() is completed the DispatchQueue object can be destructed.
I will use detachThread() in ~DispatchQueue() instead of pthread_join.
Thank you for your advice~

[Gyuyoung Kim]

(In reply to comment #13)
> (From update of attachment 239469 [details])
> DispatchQueue can be destroyed at the ThreadContext::function because the DispatchQueue::ThreadContext has a reference of the DispatchQueue instance. Joining the thread at the DispatchQueue destructor can lead a infinite waiting of the created thread.

Nice review Byungwoo ! I missed that DispatchQueue::ThreadContext has a reference of DispatchQueue though it looks very basic behavior.

[Byungwoo Lee]

:)

And could you please check one more?
As I mentioned before, I think that it will be better to detach the thread right after the thread was created instead of ~DispatchQueue() if it is possible. To do so, we can remove redundant m_threadID.

[Joonghun Park]

Created attachment 239780 [details]
Patch

[Byungwoo Lee]

Comment on attachment 239780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239780&action=review

> Source/WebKit2/ChangeLog:16
> +        Thus we need to destroy the thread together
> +        when DispatchQueue's dtor is called.
> +        This patch calls waitForThreadCompletion() to destroy the thread in dtor of DispatchQueue class.

These need to be updated.

[Joonghun Park]

Created attachment 239782 [details]
Patch

[Joonghun Park]

(In reply to comment #18)
> (From update of attachment 239780 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239780&action=review
> 
> > Source/WebKit2/ChangeLog:16
> > +        Thus we need to destroy the thread together
> > +        when DispatchQueue's dtor is called.
> > +        This patch calls waitForThreadCompletion() to destroy the thread in dtor of DispatchQueue class.
> 
> These need to be updated.

I revised the comment. Please check it once again.

[Byungwoo Lee]

LGTM if you verified tests with this patch.

[Joonghun Park]

(In reply to comment #21)
> LGTM if you verified tests with this patch.

I executed regression tests with this patch and there was no regression caused by this patch.

[Joonghun Park]

(In reply to comment #22)
> (In reply to comment #21)
> > LGTM if you verified tests with this patch.
> 
> I executed regression tests with this patch and there was no regression caused by this patch.

I mean, layout test.

[Gyuyoung Kim]

Comment on attachment 239782 [details]
Patch

r+ed based on Byungwoo internal review.

[WebKit Commit Bot]

Comment on attachment 239782 [details]
Patch

Clearing flags on attachment: 239782

Committed r174719: <http://trac.webkit.org/changeset/174719>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.