Ensure that file is open prior to reading from it

[ohenrik]

Fixes an issue where Aws::Plugins::S3Md5s::Handler.md5 would crash upon receiving a closed file.

We're not sure whether this is aws-sdk-ruby's responsibility, but seeing as how body is treated very much like a file (we're calling read and rewind on it already) it seems sensible to check that it's actually open prior to reading from it.

Fixes refile/refile-s3#10

[kcollignon]

+1

[trevorrowe]

I've been considering how to deal with this issue. I am not generally opposed, but as you point out, I don't know if this should be the SDK's responsibility. It seems reasonable, so I'm inclined to explore how to make this happen.

The MD5 handler is not the only place that the file is read. There are three places that come to mind, in the following order:

• The file is read & rewound to compute a MD5 hash
• The file is read & rewound to compute a version 4 signature, this happens for certain endpoints.
• The file is read as it is streamed over the HTTP connection.

MD5 computation is optional, sigv4 is too. If the goal is to accept possibly closed files, we would need to do a closed check before each of these. Either that or we need a pre-check that happens up-front and let the other handlers continue to assume that the IO object is ready to be read.

Thoughts?

[trevorrowe]

I was taking another look at this. I ran into an issue during testing. The #open method on File is private. I'm looking at another solution.

[trevorrowe]

This should go out some time next week. I added logic to the incoming request parameter converter. If it receives a closed file or tempfile it will open a new file in read-binary mode. Thanks for your patience.

[Senjai]

Shouldn't we raise an exception here? In what scenarios would we passing a closed file to aws be okay?

[trevorrowe]

@Senjai This issue has cropped up multiple times. Yes, it should be reasonable to require an open file. There are however many different tools and libraries that use the SDK for uploading files. It is not always straight forward for a user to track down the tool that is "responsible" for closing the file.

To make things easier for the user, this patch makes it easier on the end user:
https://github.com/aws/aws-sdk-ruby/blob/master/aws-sdk-core/lib/aws-sdk-core/param_converter.rb#L113-L121

The SDK will not attempt to open the closed file, instead is uses the file to get a path. This is the same as if the user had passed in the path to the file as a string.

[Senjai]

@trevorrowe Ah, gotcha. 👍 on that approach.