IoError for NamedTempFile not passed by reference for FsBuilder.path

[rogusdev]

When using the FsBuilder.path, which is used by ByteStream::from_path, there can very easily be an unexpected error, that seems like it should be caught by the compiler rather than as a run time error.

A NamedTempFile argument must be passed by reference (i.e. with a leading &). If you remove the &, ByteStream::from_path (and FsBuilder.path) will accept the argument, but immediately error out with an IoError. You can demo this trivially with the existing tests, e.g. remove the & from &file on this line:

smithy-rs/rust-runtime/aws-smithy-types/src/byte_stream/bytestream_util.rs

Line 323 in 23cdff1

.path(&file)

Then running the tests with:

cargo test byte_stream::bytestream_util --features rt-tokio,http-body-0-4-x,hyper-0-14-x

Will lead to this error:

---- byte_stream::bytestream_util::test::path_based_bytestreams_with_builder stdout ----
thread 'byte_stream::bytestream_util::test::path_based_bytestreams_with_builder' panicked at aws-smithy-types/src/byte_stream/bytestream_util.rs:328:14:
called `Result::unwrap()` on an `Err` value: Error { kind: IoError(Os { code: 2, kind: NotFound, message: "No such file or directory" }) }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Which seems very misleading, at best, and hopefully should be preventable at the compiler level by more appropriate typing -- or else adjusting as needed within the FsBuilder.path (or build) function.

Thanks!

[rcoh]

I don't know if there's really a great way to do this. When working with temp files you need keep them alive as long as you want the file to readable since they will be removed on Drop. Open to suggestions though. Seems like it could also be a clippy lint.

[rogusdev]

This might be a nuance of Rust I am unfamiliar with, so could you help clarify for me:

smithy-rs/rust-runtime/aws-smithy-types/src/byte_stream/bytestream_util.rs

Line 125 in 23cdff1

pub fn path(mut self, path: impl AsRef<std::path::Path>) -> Self {

Receives an AsRef<Path> that then gets stored such that, I assume, the NamedTempFile has transferred ownership when not passed as a reference -- and then been dropped. Whereas, if it does go as a reference, it continues to live until the end of the block that calls the Fsbuilder.build, is that correct?

If so, yeah, I can see the conundrum is that the temp file really has disappeared by then. I wish we had specialization with operator overloading, ha! But really I just wish we could alert some sort of "hey, be aware that maybe you just lost your temp file" warning, but idk.

If it is something other than that, I'd love to understand it better, and maybe that will help.

This is just such an easy gotcha to get caught on, and it took me quite a while to debug -- and the solution for me was simply to find the test cases to realize what I was missing, which does not seem like a high visibility insight, unfortunately.

[rcoh]

Receives an AsRef that then gets stored such that, I assume, the NamedTempFile has transferred ownership when not passed as a reference -- and then been dropped. Whereas, if it does go as a reference, it continues to live until the end of the block that calls the Fsbuilder.build, is that correct?

Yeah that's correct. I've personally been caught up by NamedTempFile a number of times (although not on this particular API).

It's definitely possible to add a Clippy lint for this. I'm surprised one hasn't been created already, honestly.