fix: handle concurrent rustls cryptoprovider initialization

[reez]

Fixes #216
References bitcoindevkit/bdk-dart#103

When multiple SSL clients are created concurrently more than one caller can observe that no process-default rustls CryptoProvider is installed. This keeps the existing install path, but if install_default() fails, it re-checks whether a default provider is now present and treats that as success.

I tried to look for broader context/discussion in things like #171 (comment) and #171 (comment). This keeps the existing get_default() guard behavior, but also handles the concurrent "first use" case where another caller installs the provider after the initial check.

[oleonardolima]

@reez I pushed 1ca8828 to fix the CI.

[luisschwab]

ACK 3ea825a

[reez]

@reez I pushed 1ca8828 to fix the CI.

cool appreciated 👍

[caarloshenriq]

ACK 3ea825a

Tried to reproduce #216 manually via concurrent Client::from_config calls, but couldn't trigger it reliably over a real network connection, timing is too noisy. I think we should add a regression test for this case.

Suggestion: extract the install logic into its own function, then hit it directly with many threads via a Barrier, no network or TLS involved. Something like:

fn install_crypto_provider() -> Result<(), Error> {
    if rustls::crypto::CryptoProvider::get_default().is_none() {
        #[cfg(all(feature = "rustls", not(feature = "rustls-ring")))]
        rustls::crypto::CryptoProvider::install_default(
            rustls::crypto::aws_lc_rs::default_provider(),
        )
        .or_else(|_| {
            rustls::crypto::CryptoProvider::get_default()
                .map(|_| ())
                .ok_or_else(|| {
                    Error::CouldNotCreateConnection(rustls::Error::General(
                        "Failed to install CryptoProvider".to_string(),
                    ))
                })
        })?;

        #[cfg(feature = "rustls-ring")]
        rustls::crypto::CryptoProvider::install_default(
            rustls::crypto::ring::default_provider(),
        )
        .or_else(|_| {
            rustls::crypto::CryptoProvider::get_default()
                .map(|_| ())
                .ok_or_else(|| {
                    Error::CouldNotCreateConnection(rustls::Error::General(
                        "Failed to install CryptoProvider".to_string(),
                    ))
                })
        })?;
    }
    Ok(())
}

#[test]
fn test_concurrent_crypto_provider_install() {
    use std::sync::{Arc, Barrier};
    use std::thread;

    let n = 64;
    let barrier = Arc::new(Barrier::new(n));
    let handles: Vec<_> = (0..n)
        .map(|_| {
            let barrier = Arc::clone(&barrier);
            thread::spawn(move || {
                barrier.wait();
                install_crypto_provider()
            })
        })
        .collect();

    for h in handles {
        assert!(h.join().unwrap().is_ok());
    }
}

[reez]

ACK 3ea825a

Tried to reproduce #216 manually via concurrent Client::from_config calls, but couldn't trigger it reliably over a real network connection, timing is too noisy. I think we should add a regression test for this case.

Suggestion: extract the install logic into its own function, then hit it directly with many threads via a Barrier, no network or TLS involved. Something like:

fn install_crypto_provider() -> Result<(), Error> {
    if rustls::crypto::CryptoProvider::get_default().is_none() {
        #[cfg(all(feature = "rustls", not(feature = "rustls-ring")))]
        rustls::crypto::CryptoProvider::install_default(
            rustls::crypto::aws_lc_rs::default_provider(),
        )
        .or_else(|_| {
            rustls::crypto::CryptoProvider::get_default()
                .map(|_| ())
                .ok_or_else(|| {
                    Error::CouldNotCreateConnection(rustls::Error::General(
                        "Failed to install CryptoProvider".to_string(),
                    ))
                })
        })?;

        #[cfg(feature = "rustls-ring")]
        rustls::crypto::CryptoProvider::install_default(
            rustls::crypto::ring::default_provider(),
        )
        .or_else(|_| {
            rustls::crypto::CryptoProvider::get_default()
                .map(|_| ())
                .ok_or_else(|| {
                    Error::CouldNotCreateConnection(rustls::Error::General(
                        "Failed to install CryptoProvider".to_string(),
                    ))
                })
        })?;
    }
    Ok(())
}

#[test]
fn test_concurrent_crypto_provider_install() {
    use std::sync::{Arc, Barrier};
    use std::thread;

    let n = 64;
    let barrier = Arc::new(Barrier::new(n));
    let handles: Vec<_> = (0..n)
        .map(|_| {
            let barrier = Arc::clone(&barrier);
            thread::spawn(move || {
                barrier.wait();
                install_crypto_provider()
            })
        })
        .collect();

    for h in handles {
        assert!(h.join().unwrap().is_ok());
    }
}

Here's kinda my thinking/approach and what I did: reproduced the CryptoProvider initialization race locally by ensuring that several threads all found no default provider before any of them attempted to install one (with the old logic, the threads that lost the race returned Failed to install CryptoProvider... with this change, they all succeeded. And this way forced the timing behind the reported failure instead of having to rely on concurrent client connections to happen to hit it).

Because the provider is shared across the entire test process, starting the threads together wouldn’t guarantee that the test exercises the race so I verified the behavior with the controlled local reproduction, but turning that into a deterministic test would require broader changes to the larger testing setup to isolate the provider state shared by every test in the process and control the initialization timing.

Thanks for the ack and checking this out.

[caarloshenriq]

Here's kinda my thinking/approach and what I did: reproduced the CryptoProvider initialization race locally by ensuring that several threads all found no default provider before any of them attempted to install one (with the old logic, the threads that lost the race returned Failed to install CryptoProvider... with this change, they all succeeded. And this way forced the timing behind the reported failure instead of having to rely on concurrent client connections to happen to hit it).

Because the provider is shared across the entire test process, starting the threads together wouldn’t guarantee that the test exercises the race so I verified the behavior with the controlled local reproduction, but turning that into a deterministic test would require broader changes to the larger testing setup to isolate the provider state shared by every test in the process and control the initialization timing.

Thanks for the ack and checking this out.

Makes sense, thanks for the detailed explanation.

[caarloshenriq]

tACK 3ea825a