From 02174e6402f7ffdb813a6aa40d8e1851175d66e5 Mon Sep 17 00:00:00 2001 From: "Stephen M. Coakley" Date: Sat, 23 May 2020 19:21:43 -0500 Subject: [PATCH 1/4] Experimental use of constructor functions to call init As mentioned in #333, this is a potentially helpful addition that ensures that curl is initialized on the main thread by using constructor functions that get called by the OS before the current program's `main` is called. This has the advantage that, assuming you are on one of the supported platforms, `init()` will be called safely, correctly, and automatically without concerning the user about the gotchas. This does have some disadvantages: - Constructor functions are always invoked, which means that simply including curl can slow down the startup of a user's program even if curl is only conditionally used, or used later in program execution. - On platforms without a constructor implementation, the user still needs to initialize curl on the main thread. All the common platforms are covered though, so maybe this is a niche scenario. There's no additional runtime cost to this implementation except on platforms without a constructor, where we do an extra atomic swap that isn't necessary. This is probably fixable with additional conditional compilation. --- src/lib.rs | 64 ++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 21 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index b9c0c01186..348f1919be 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -64,7 +64,7 @@ extern crate schannel; use std::ffi::CStr; use std::str; -use std::sync::Once; +use std::sync::{atomic::{AtomicBool, Ordering}, Once}; pub use error::{Error, FormError, MultiError, ShareError}; mod error; @@ -81,31 +81,53 @@ mod panic; /// It's not required to call this before the library is used, but it's /// recommended to do so as soon as the program starts. pub fn init() { + /// An exported constructor function. On supported platforms, this will be + /// invoked automatically before the program's `main` is called. + #[cfg_attr(any(target_os = "linux", target_os = "android"), link_section = ".init_array")] + #[cfg_attr(target_os = "freebsd", link_section = ".init_array")] + #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")] + #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")] + static INIT_CTOR: extern "C" fn() = init_inner; + + /// Used to prevent concurrent calls when initializing after `main` starts. static INIT: Once = Once::new(); - INIT.call_once(|| { - platform_init(); - unsafe { - assert_eq!(curl_sys::curl_global_init(curl_sys::CURL_GLOBAL_ALL), 0); - } - // Note that we explicitly don't schedule a call to - // `curl_global_cleanup`. The documentation for that function says - // - // > You must not call it when any other thread in the program (i.e. a - // > thread sharing the same memory) is running. This doesn't just mean - // > no other thread that is using libcurl. + // We invoke our init function through our static to ensure the symbol isn't + // optimized away due to a rustc bug: + // https://github.com/rust-lang/rust/issues/47384 + INIT.call_once(|| INIT_CTOR()); + + #[cfg_attr(any(target_os = "linux", target_os = "android"), link_section = ".text.startup")] + extern "C" fn init_inner() { + // We can't rely on just a `Once` for initialization, because + // constructor functions are called before the Rust runtime starts, and + // `Once` relies on facilities provided by the Rust runtime. So we + // additionally protect against multiple initialize calls with this + // atomic. // - // We can't ever be sure of that, so unfortunately we can't call the - // function. - }); + // If the current platform does not support constructor functions, then + // we will rely on the `Once` to call initialization. + static IS_INIT: AtomicBool = AtomicBool::new(false); - #[cfg(need_openssl_init)] - fn platform_init() { - openssl_sys::init(); - } + if !IS_INIT.compare_and_swap(false, true, Ordering::Acquire) { + #[cfg(need_openssl_init)] + openssl_sys::init(); + + unsafe { + assert_eq!(curl_sys::curl_global_init(curl_sys::CURL_GLOBAL_ALL), 0); + } - #[cfg(not(need_openssl_init))] - fn platform_init() {} + // Note that we explicitly don't schedule a call to + // `curl_global_cleanup`. The documentation for that function says + // + // > You must not call it when any other thread in the program (i.e. + // > a thread sharing the same memory) is running. This doesn't just + // > mean no other thread that is using libcurl. + // + // We can't ever be sure of that, so unfortunately we can't call the + // function. + } + } } unsafe fn opt_str<'a>(ptr: *const libc::c_char) -> Option<&'a str> { From f2c07bc13323025fc9d6a5011a3ae72debfc0552 Mon Sep 17 00:00:00 2001 From: "Stephen M. Coakley" Date: Mon, 25 May 2020 00:15:15 -0500 Subject: [PATCH 2/4] Micro-optimization Ensure subsequent init calls are compiled down to a single atomic compare-and-swap. --- src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 348f1919be..ad09806432 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -80,6 +80,7 @@ mod panic; /// /// It's not required to call this before the library is used, but it's /// recommended to do so as soon as the program starts. +#[inline] pub fn init() { /// An exported constructor function. On supported platforms, this will be /// invoked automatically before the program's `main` is called. @@ -93,8 +94,7 @@ pub fn init() { static INIT: Once = Once::new(); // We invoke our init function through our static to ensure the symbol isn't - // optimized away due to a rustc bug: - // https://github.com/rust-lang/rust/issues/47384 + // optimized away by a bug: https://github.com/rust-lang/rust/issues/47384 INIT.call_once(|| INIT_CTOR()); #[cfg_attr(any(target_os = "linux", target_os = "android"), link_section = ".text.startup")] From c75a60c447a515e58f0f6aec2b072d410c726dbf Mon Sep 17 00:00:00 2001 From: "Stephen M. Coakley" Date: Wed, 27 May 2020 11:03:18 -0500 Subject: [PATCH 3/4] Consolidate ELF attribute --- src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index ad09806432..23d0a89e4e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -84,8 +84,7 @@ mod panic; pub fn init() { /// An exported constructor function. On supported platforms, this will be /// invoked automatically before the program's `main` is called. - #[cfg_attr(any(target_os = "linux", target_os = "android"), link_section = ".init_array")] - #[cfg_attr(target_os = "freebsd", link_section = ".init_array")] + #[cfg_attr(any(target_os = "linux", target_os = "freebsd", target_os = "android"), link_section = ".init_array")] #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")] #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")] static INIT_CTOR: extern "C" fn() = init_inner; From d4c004196ad1ca15dd41df4ccd7dad5f3e219901 Mon Sep 17 00:00:00 2001 From: "Stephen M. Coakley" Date: Sat, 30 May 2020 13:39:39 -0500 Subject: [PATCH 4/4] Reducing to just a Once is acceptable Since it is not possible for the `Once` to have contention in a constructor, it is safe to use it. (Under contention, `Once` requires `std::thread` machinery to work, which is not yet available before `main`.) Also update documentation on initialization. --- src/lib.rs | 62 +++++++++++++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 24 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 23d0a89e4e..9f2e50ea9b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -46,6 +46,12 @@ //! There is a large number of releases for libcurl, all with different sets of //! capabilities. Robust programs may wish to inspect `Version::get()` to test //! what features are implemented in the linked build of libcurl at runtime. +//! +//! # Initialization +//! +//! The underlying libcurl library must be initialized before use and has +//! certain requirements on how this is done. Check the documentation for +//! [`init`] for more details. #![deny(missing_docs, missing_debug_implementations)] #![doc(html_root_url = "https://docs.rs/curl/0.4")] @@ -64,7 +70,7 @@ extern crate schannel; use std::ffi::CStr; use std::str; -use std::sync::{atomic::{AtomicBool, Ordering}, Once}; +use std::sync::Once; pub use error::{Error, FormError, MultiError, ShareError}; mod error; @@ -78,37 +84,41 @@ mod panic; /// Initializes the underlying libcurl library. /// -/// It's not required to call this before the library is used, but it's -/// recommended to do so as soon as the program starts. +/// The underlying libcurl library must be initialized before use, and must be +/// done so on the main thread before any other threads are created by the +/// program. This crate will do this for you automatically in the following +/// scenarios: +/// +/// - Creating a new [`Easy`][easy::Easy] or [`Multi`][multi::Multi] handle +/// - At program startup on Windows, macOS, Linux, Android, or FreeBSD systems +/// +/// This should be sufficient for most applications and scenarios, but in any +/// other case, it is strongly recommended that you call this function manually +/// as soon as your program starts. +/// +/// Calling this function more than once is harmless and has no effect. #[inline] pub fn init() { + /// Used to prevent concurrent or duplicate initialization. + static INIT: Once = Once::new(); + /// An exported constructor function. On supported platforms, this will be /// invoked automatically before the program's `main` is called. - #[cfg_attr(any(target_os = "linux", target_os = "freebsd", target_os = "android"), link_section = ".init_array")] + #[cfg_attr( + any(target_os = "linux", target_os = "freebsd", target_os = "android"), + link_section = ".init_array" + )] #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")] #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")] static INIT_CTOR: extern "C" fn() = init_inner; - /// Used to prevent concurrent calls when initializing after `main` starts. - static INIT: Once = Once::new(); - - // We invoke our init function through our static to ensure the symbol isn't - // optimized away by a bug: https://github.com/rust-lang/rust/issues/47384 - INIT.call_once(|| INIT_CTOR()); - - #[cfg_attr(any(target_os = "linux", target_os = "android"), link_section = ".text.startup")] + /// This is the body of our constructor function. + #[cfg_attr( + any(target_os = "linux", target_os = "android"), + link_section = ".text.startup" + )] extern "C" fn init_inner() { - // We can't rely on just a `Once` for initialization, because - // constructor functions are called before the Rust runtime starts, and - // `Once` relies on facilities provided by the Rust runtime. So we - // additionally protect against multiple initialize calls with this - // atomic. - // - // If the current platform does not support constructor functions, then - // we will rely on the `Once` to call initialization. - static IS_INIT: AtomicBool = AtomicBool::new(false); - - if !IS_INIT.compare_and_swap(false, true, Ordering::Acquire) { + INIT.call_once(|| { #[cfg(need_openssl_init)] openssl_sys::init(); @@ -125,8 +135,12 @@ pub fn init() { // // We can't ever be sure of that, so unfortunately we can't call the // function. - } + }); } + + // We invoke our init function through our static to ensure the symbol isn't + // optimized away by a bug: https://github.com/rust-lang/rust/issues/47384 + INIT_CTOR(); } unsafe fn opt_str<'a>(ptr: *const libc::c_char) -> Option<&'a str> {