Bug 32864 - [CURL] Mutex objects are not required to protect libcurl
Summary: [CURL] Mutex objects are not required to protect libcurl
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-22 03:19 PST by Kwang Yul Seo
Modified: 2013-07-16 00:44 PDT (History)
5 users (show)

See Also:


Attachments
Don't protect libcurl's shared data with mutex (3.38 KB, patch)
2009-12-22 03:33 PST, Kwang Yul Seo
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2009-12-22 03:19:40 PST
There is a known bug in libcurl's threaded resolver in Win32:

crash using libcurl 7.19 multi interface
http://curl.haxx.se/mail/lib-2009-03/0092.html

In the past, Kevin Watters suggested a solution to fix the crash by protecting libcurl's shared data with mutex.

curl DNS lookup occurs on multiple threads but storage is not threadsafe
https://bugs.webkit.org/show_bug.cgi?id=28920

I'm afraid that this is not the correct way to solve the problem. As mentioned in the libcurl's mailing list, this bug is caused by the internal bug in libcurl's threaded DNS resolver.

According to the libcurl's manual:

"Since you can use this share from multiple threads, and libcurl has no internal thread synchronization, you must provide mutex callbacks if you're using this multi-threaded. "

Because ResourceHandleManager is single-threaded with multi interface, we don't need thread synchronization at all.

The right way to fix this problem is to use the synchronous resolver or c-ares resolver in libcurl build as mentioned in the mailing list. The current ResourceHandleManager code causes unnecessary locking overhead in other platforms.
Comment 1 Kwang Yul Seo 2009-12-22 03:23:23 PST
With multi interface, the DNS data is automatically shared. So the following line is also unnecessary.

curl_share_setopt(m_curlShareHandle, CURLSHOPT_SHARE, CURL_LOCK_DATA_DNS);
Comment 2 Kwang Yul Seo 2009-12-22 03:33:05 PST
Created attachment 45379 [details]
Don't protect libcurl's shared data with mutex
Comment 3 WebKit Review Bot 2009-12-22 03:35:19 PST
style-queue ran check-webkit-style on attachment 45379 [details] without any errors.
Comment 4 Kwang Yul Seo 2009-12-22 04:03:01 PST
Another solution for this problem is keep the current workaround with platform-specific guards so that other platforms are not affected.

We can get rid of mutex callbacks once the problem is fixed in libcurl itself.

I see the work in progress:

http://curl.haxx.se/mail/lib-2009-09/0316.html

http://sourceforge.net/tracker/index.php?func=detail&aid=2884791&group_id=976&atid=100976
Comment 5 Eric Seidel (no email) 2010-01-05 13:53:00 PST
What will removing this code do?

Will it expose a crasher?  Will it make things faster?  Will it remove dead code?

Your changelog is nice, but I'm still not clear on the purpose of this change.  You explained more What than Why (both are useful).
Comment 6 Eric Seidel (no email) 2010-01-26 14:31:17 PST
Comment on attachment 45379 [details]
Don't protect libcurl's shared data with mutex

No response in over 2 weeks.  r-.  Please re-flag when you've answered my question.
Comment 7 Kwang Yul Seo 2013-07-16 00:44:17 PDT
Not valid anymore.