Breaking changes in v2.1.2

[tyilo]

The following code works when using v2.1.1 of amqprs, but fails when using v2.1.2:

[package]
name = "amqprs-test"
version = "0.1.0"
edition = "2024"

[dependencies]
amqprs = { version = "=2.1.1", features = ["tls", "urispec"] } # or: version = "=2.1.2"
tokio = { version = "1.45.1", features = ["full"] }

use std::error::Error;

use amqprs::connection::{Connection, OpenConnectionArguments};

#[tokio::main]
async fn main() -> Result<(), Box<dyn Error>> {
    let url = std::env::var("AMQP_URL")?;
    let args = OpenConnectionArguments::try_from(url.as_str())?;
    let connection = Connection::open(&args).await?;
    println!("{}", connection.connection_name());

    Ok(())
}

Error when using v2.1.2:

thread 'main' panicked at /home/tyilo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/rustls-0.23.28/src/crypto/mod.rs:249:14:
no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Seems to be a similar situation as in #143 and I think v2.1.2 should be yanked.

[gftea]

I add @cdbennett into discussion for this, the recommended way forward I see is to add install_default() from now

[cdbennett]

Basically in amqprs 2.1.1, we have a dependency bug that forces pulling in both the ring and aws-lc-rs crate. In amqprs 2.1.2 it gives the application binary crate's Cargo.toml file the freedom to choose one or the other of these crypto crates.

So you're correct that it breaks the execution of this program, but it fixes other programs that must be able to choose one or the other of these crypto crates.

Instead of yanking 2.1.2, I think I could have provided a more helpful release note to give users an example of how the TLS crate ambiguity should be fixed.

With you demo using amqprs 2.1.1 (successfully builds/runs, it's unclear which crypto crate is actually used at runtime however, as both are included and neither is explicitly selected with CryptoProvider::install_default()):

amqprs-test v0.1.0 (/.../amqprs-test)
├── amqprs v2.1.1
...
│   ├── rustls-webpki v0.102.8
│   │   ├── ring v0.17.14
...
│   ├── tokio-rustls v0.26.2
│   │   ├── rustls v0.23.29
│   │   │   ├── aws-lc-rs v1.13.2
│   │   │   │   ├── aws-lc-sys v0.30.0
...
│   │   │   ├── rustls-webpki v0.103.4
│   │   │   │   ├── aws-lc-rs v1.13.2 (*)
│   │   │   │   ├── rustls-pki-types v1.12.0 (*)
│   │   │   │   └── untrusted v0.9.0
...

So here's the odd thing, we have two versions of rustls-webpki, both 0.102 and 0.103 and each pulls in a different crypto crate! Not good.

Solution:

Change the application's Cargo.toml to add a rustls dependency selecting which crypto crate to use via feature flag. Example: Comment/uncomment one of the two rustls = lines to select the crypto crate to use:

[package]
name = "amqprs-test"
version = "0.1.0"
edition = "2024"

[dependencies]
amqprs = { version = "=2.1.2", features = ["tls", "urispec"] }
tokio = { version = "1.45.1", features = ["full"] }

# This line for aws-lc:
rustls = { version = "0.23", features = ["aws-lc-rs"], default-features = false }

# This line for ring:
# rustls = { version = "0.23", features = ["ring"], default-features = false }

You actually don't need to call install_default(), if exactly one crypto crate is linked in. But an explicit call is recommended by rustls as of 0.23. Here's their comment in the release notes:

Support for process-wide selection of CryptoProviders. See the documentation. Note that callers of ClientConfig::builder(), ServerConfig::builder(), WebPkiServerVerifier::builder() and WebPkiClientVerifier::builder() must now ensure that the crate's features are unambiguous or explicitly select a process-level provider using CryptoProvider::install_default(). Otherwise, these calls will panic with:
no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point
We recommend that libraries rely on the process-level provider by default, and that applications use this new API to select the provider they wish to use.

[tyilo]

The exact same argument as #143 (comment) applies here:

My application is not using rustls (directly), but amqprs is. The update from 1.6 to 1.7 clearly can't be done without any changes to my code and should thus be a breaking change.

Same applies here with version 2.1.1 and 2.1.2. It is a breaking change, it breaks my code! Consider yanking 2.1.2 and releasing a 3.0.0 with this feature or releasing a version 2.1.3 which is reverts the changes in 2.1.2 and wait for 3.0.0 for the 2.1.2 changes.

[tyilo]

@gftea Do you feel like it was a mistake to yank 1.7.0 and bump the version to 2.0.0?

If not, why not do something similar now?

[gftea]

@tyilo, my recommendation is as @cdbennett , due to large project, dependency conflicts issue are troublesome if encountered.
After being used for a while, I think we should ensure this is applied now, The fix just add CryptoProvider::install_default() if you do not care about which provider to use.

[gftea]

@cdbennett , may you help to add this to readme file? In the test cases and examples, we already have all fixed with CryptoProvider::install_default()

[michaelklishin]

@tyilo it's absolutely ridiculous to bump a major over something like this. This is a bug fix to many, some bug fixes, sadly, cannot be entirely backwards compatible.

The code change required on your side is fairly trivial, either make the change or move back to an earlier patch release.

You get this software for free and have never contributed to it, so be nice and appreciative or go maintain your own fork.

[michaelklishin]

@gftea the optimal thing to do here is to bump the minor (it is a minor breaking change) but that would leave the latest patch of the previous minor series affected, so I guess yanking it make some sense.

External dependency changes in patch releases are very likely to cause this kind of discussions, so don't be shy about bumping the minor version, even if the idea is effectively a bug (or compatibility) fix. That usually silences even the most fanatical desciples of the Church of Strict SemVer™ and allows you to not do ridiculous things like bumping the major version for a minor change (all things considered).

[tyilo]

@tyilo it's absolutely ridiculous to bump a major over something like this. This is a bug fix to many, some bug fixes, sadly, cannot be entirely backwards compatible.

The code change required on your side is fairly trivial, either make the change or move back to an earlier patch release.

I don't see why it is such a big deal to bump the major version. You broke people's code, bump the major version and move on...

You get this software for free and have never contributed to it, so be nice and appreciative or go maintain your own fork.

I have 3 merged PR's, so what do you mean? https://github.com/gftea/amqprs/pulls?q=is%3Apr%20author%3Atyilo%20is%3Aclosed

[michaelklishin]

I don't see why it is such a big deal to bump the major version. You broke people's code, bump the major version and move on

Because if you do that for even the smallest breaking change, the whole idea of a major release will become meaningless. Let's bump the major version every single release, what's the big deal?

How can a user then tell if a series actually has meaningful API changes? Not something you can fix in 15 minutes and move on but something that can require hours or even person-days worth of effort. You can't.

Somewhere in the process of re-reading and re-citing the SemVer bible, a lot of common sense was lost. Striving to not introduce major breaking changes without bumping the major version is a great goal. Dogmatically repeating that no patch or minor release ever should introduce any breaking changes for anyone is a whole different story.

[tyilo]

Because if you do that for even the smallest breaking change, the whole idea of a major release will become meaningless. Let's bump the major version every single release, what's the big deal?

Just a reply to your comment, not referring to this issue or amqprs in general:
Then don't do small breaking changes all the time? How hard can that be?

[cdbennett]

@tyilo, my recommendation is as @cdbennett , due to large project, dependency conflicts issue are troublesome if encountered. After being used for a while, I think we should ensure this is applied now, The fix just add CryptoProvider::install_default() if you do not care about which provider to use.

Slight clarification: so in the sample code in this issue, after bumping amqprs to v2.1.2, Cargo does not pull in any TLS implementation, neither ring nor aws-lc. Proposed solution is to add a dependency on rustls either with its default features, or no-default-features and specify a feature for which TLS implementation to use.

In amqprs, we might be able to improve the situation by tweaking our default features set and thus having a project "just work" without specifying any TLS implementation. Like it was in 1.x. Example: have a feature aws-lc-rs for amqprs (and put it in the default features), which enables the rustls crate's aws-lc-rs feature. Then applications can select a different TLS crate (ring) by disabling amqprs default features and specifying ring as a feature.

The problem I see with that approach is that it will be too easy for other library crates to depend on amqprs's default features, and then you can end up with applications being forced to consume that default TLS crate even if they want to use another.

[cdbennett]

OK, there are some strong opinions here. Let's be cool and keep things in perspective.

It's unfortunate when fixing one bug can break something related. But in the scope of things it's just adding a few lines in Cargo.toml to fix it. Also, remember that the TLS crate feature/dependency mess related to rustls has wider impact than just amqprs as well.

Is there a real application or dependent crate that's still broken as a result of the v2.1.2 change? There's multiple ways it could be solved, but we do have a path forward that shouldn't be too burdensome on anyone.

[tyilo]

Is there a real application or dependent crate that's still broken as a result of the v2.1.2 change? There's multiple ways it could be solved, but we do have a path forward that shouldn't be too burdensome on anyone.

We had a real crash of a service in production due to the v2.1.2 change. We have of course fixed the service (for now by pinning v2.1.1), but we might have other services that could experience the same problem when doing a simple patch upgrade of its dependencies.

[cdbennett]

One thing I like about Rust is its explicitness. I.e. the code clearly tells you what it does, and almost everything can be statically verified at compile time. As opposed to another software environment I have all too much experience with, Java Spring Boot... in Spring Boot you can't tell until runtime if the magical framework will actually find all the right classes to configure your application, or it will crash at startup with 5000 lines of stack traces.

Here, we have seen how magic (a lack of explicitness) in Rust code (in this case of rustls and the TLS provider crates) produces a bad experience just like Spring Boot can. I sympathize with your frustration. Partly it is rustls's magical automatic detection and registration of the TLS provider, and also it seems like the Rust feature flags system is often pushed too far and it does cause problems like this.

Fortunately we have a path forward: most importantly, if application code explicitly refers to the crate implementation that should be used (calling CryptoProvider::install_default()), it will make sure that dependency problems are caught at compile time and not runtime.