Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

curl_global_init(): make it thread-safe #8680

Closed
wants to merge 2 commits into from

Conversation

tguillem
Copy link
Contributor

@tguillem tguillem commented Apr 5, 2022

Making curl_global_init() thread-safe can be very useful for libraries that can't control what other libraries (or the main application) are doing with Curl. That way, curl_global_init() doesn't have to be called at the very beginning of the process and can be called anytime (from another library init function, that is run later, for example).

PS: I didn't know that number to choose for the new test (3026).

@tguillem tguillem force-pushed the curl-global-init-ts branch 2 times, most recently from 841d2b4 to 6e5c2cd Compare Apr 5, 2022
lib/easy.c Outdated Show resolved Hide resolved
@jay
Copy link
Member

@jay jay commented Apr 6, 2022

Also see #4064

@bagder
Copy link
Member

@bagder bagder commented Apr 6, 2022

Would it be possible to enable pthread and win32 threads by default or via another configure option (like --init-thread-safe)?

Sure it would be possible. It would imagine that check should be enabled by default.

But I would also like to see the #4064 and #5017 approaches considered properly, as they might be able to make us reach this goal without using a thread library.

lib/easy.c Outdated Show resolved Hide resolved
@tguillem
Copy link
Contributor Author

@tguillem tguillem commented Apr 6, 2022

Would it be possible to enable pthread and win32 threads by default or via another configure option (like --init-thread-safe)?

Sure it would be possible. It would imagine that check should be enabled by default.

But I would also like to see the #4064 and #5017 approaches considered properly, as they might be able to make us reach this goal without using a thread library.

On Windows, we could do implement a lock via Interlocked (Atomic like) and WaitOnAddress (futex like) :
There won't be protection against lock convoy or spinlock optimization, but I don't think we care if we use this lock only for the global init (it's very unlikely that init/cleanup are called a lot).

On Linux, we could do the same with atomics and futexes, but futuxes are not available on all POSIX systems (we could fall back to a wait loop but I'm not very fan). That's why I would prefer using pthread.

@tguillem tguillem force-pushed the curl-global-init-ts branch 6 times, most recently from 6597e58 to 8b65933 Compare Apr 8, 2022
lib/easy.c Show resolved Hide resolved
@tguillem
Copy link
Contributor Author

@tguillem tguillem commented Apr 14, 2022

Would it be possible to enable pthread and win32 threads by default or via another configure option (like --init-thread-safe)?

Sure it would be possible. It would imagine that check should be enabled by default.

But I would also like to see the #4064 and #5017 approaches considered properly, as they might be able to make us reach this goal without using a thread library.

If a program need a thread-safe init for curl, then it is likely already linking with the pthread library (on posix). I prefer the mutex approach to keep it simple since atomics or spinlocks are too often wrongly used (lock convoy, fairness, active wait).

For Windows, the SRWLock is a good a light solution too.

@tguillem
Copy link
Contributor Author

@tguillem tguillem commented Apr 27, 2022

Hello,

I just proposed an alternative that doesn't need pthread: 7417fd4

We still use SRWLOCK for Windows since it is simpler that way (don't need to use Windows Atomic API, and it is very likely to be a spinlock internally).

Use a posix pthread or a Windows SRWLOCK to lock curl_global_init*() and
curl_global_cleanup().
This flag can be used to make sure that curl_global_init() is
thread-safe.

This can be useful for libraries that can't control what other
dependencies are doing with Curl.
bagder
bagder approved these changes Jun 7, 2022
Copy link
Member

@bagder bagder left a comment

👍 this is good stuff. Let's do this.

lib/easy_lock.h Show resolved Hide resolved
@bagder bagder closed this in 23af112 Jun 7, 2022
bagder pushed a commit that referenced this issue Jun 7, 2022
This flag can be used to make sure that curl_global_init() is
thread-safe.

This can be useful for libraries that can't control what other
dependencies are doing with Curl.

Closes #8680
@bagder
Copy link
Member

@bagder bagder commented Jun 7, 2022

Thanks!

@mback2k
Copy link
Member

@mback2k mback2k commented Jun 13, 2022

I don't think this PR was sufficiently tested as the later merge of #8976 with 26 failed CI runs unfortunately shows.

@bagder
Copy link
Member

@bagder bagder commented Jun 13, 2022

We saw zero interest from anyone to do more tests and we had no indication that this was not good enough on Windows. I think we merged it fine. We keep suffering on Windows because we have too few people around who know about and care for Windows (before things are merged), and the Windows CI jobs are flaky.

The test seems to have problems in itself, but does it show that this code isn't good enough and doesn't do what it's set out to do?

@mback2k
Copy link
Member

@mback2k mback2k commented Jun 13, 2022

The Windows CI is far less flaky nowadays. Most failures are now caused by the underlying infrastructure or memory issues, not the tests themselves anymore. So, if you see more than 1 CI failing it is definitely worth checking.

I am sorry, but I personally can't keep up with this all the time although I am trying very hard. I tried to make the CI as stable as possible and still do, and then people just ignoring it again makes me question the whole purpose.

I don't know if this code without the test is broken, I just focused on the test last night. I will try to replicate a Windows specific test with CreateThread.

@mback2k
Copy link
Member

@mback2k mback2k commented Jun 13, 2022

At least I would determine that a test pass rate of 99.91 % on Azure CI can be considered quite stable, but unfortunately this does not include CI and test runs that timed out:
image

@bagder
Copy link
Member

@bagder bagder commented Jun 13, 2022

Most failures are now caused by the underlying infrastructure or memory issues, not the tests themselves anymore

The net result is still that there is virtually zero chance of all CI jobs to run green on Windows which makes it very easy to miss when real issues occur on just a few windows builds. I know I miss such ones all the time. And I do try to look for them.

@jay
Copy link
Member

@jay jay commented Jun 13, 2022

IMO this is not any of our fault. IIRC any CI we've used has always been flaky on Windows, weird input/output issues that can't be reproduced, and a bunch of hacks we have to create to get things working. They seem to be way more sensitive to timing and available resources. And credit to Marc for his work on this, they are definitely more stable than they used to be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants