Return errors instead of panicking for Endpoint::set_endpoint #1496
Comments
|
Hello! We appreciate the concern and definitely want to keep these cases to a minimum for the reasons you outlined. I'll briefly dive into each of these below to see if we have any issues that need fixing. I think the idea for this one was that the passed in
The In this case, there will always be a next character due to the nature of string splitting. This one is safe, and there's also a
Both of these are only used for tests, so it doesn't matter if we unwrap in them. Action items from this:
Thanks! |
mfelsche
changed the title
|
I did get the panic when doing the following: const MINIO_ROOT_USER: &str = "tremor";
const MINIO_ROOT_PASSWORD: &str = "snot_badger";
const MINIO_REGION: &str = "eu-central-1";
use aws_sdk_s3::{CLient, Config, Credentials, Region, Endpoint};
let s3_config = Config::builder()
.credentials_provider(Credentials::new(
MINIO_ROOT_USER,
MINIO_ROOT_PASSWORD,
None,
None,
"Environment",
))
.region(Region::new(MINIO_REGION))
.endpoint_resolver(Endpoint::immutable(
format!("localhost:{http_port}").parse().unwrap(), // adding http:// does not make it panic anymore
))
.build();
let client = Client::from_conf(s3_config)
client.list_buckets().send().await?; // panicHere is a backtrace of the panic, so you can follow the call-graph: |
|
Another small reproducer: use aws_smithy_http::endpoint::Endpoint;
fn main() -> anyhow::Result<()> {
let mut uri = "localhost:12345".parse::<http::Uri>()?;
let endpoint = Endpoint::immutable(uri.clone()); // all good
endpoint.set_endpoint(&mut uri, None); // panic!
println!("Endpoint: {:?}", &endpoint);
Ok(())
} |
|
Thanks for digging into that! You're welcome to contribute a fix if you want to. Otherwise, we'll get to this at some point in the near future. |
|
I would be up for fixing that. What would a fix look like for you? Should the constructors |
|
I think While we're at it, it might be worth adding a constructor that accepts In places where these constructors are invoked, you can just panic in your PR and we can deal with the ripples if handling the error isn't obvious in-situ. |
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We are happy users of the rust aws_sdk but just recently found out that its underlying libraries use
.expect("...")in several places and thus will panic under some conditions. One such condition would be e.g. setting a custom endpoint with a slightly invalid URL: https://github.com/awslabs/smithy-rs/blob/e1e9a29d5db1330eb51c1e84e5c6c3acb9b4f65e/rust-runtime/aws-smithy-http/src/endpoint.rs#L84-L105There are many more places where smithy code could panic. Some of those might be legit as they might never be triggered, but some of those are actually dangerous.
In our codebase we take great care to avoid panicking constructs and we would love to have the rust aws_sdk do the same. We are ready to bite the bullet of handling additional
Result<..., ...>return types in sdk code.The text was updated successfully, but these errors were encountered: