Fix upload for large BytesIO

[sveinnfannar]

Describe your changes

Uploading a BytesIO over 4MiB results in a ValueError: I/O operation on closed file due to the stream being closed twice. Once when checking the file size and again when uploading. This PR make sure the stream is not closed when checking the file size. The PR also includes a test to reproduce the error.

Slack thread about the issue.

Changelog

[erikbern]

Nice!

[mwaskom]

Hey, super thanks for the PR and especially for the new test! I am a little concerned that simply removing the context manager will cause other problems though — this code path is also used when uploading files from disk, and I think that this change risks leaving those file handles open. I need to look a little bit more closely though — It's definitely a little trick to follow the code here.

[sveinnfannar]

You a re right @mwaskom, good catch!

How do we proceed?
I don't mind updating the PR after bouncing a few ideas with you.
If you'd rather solve this within your team at Modal that's fine with me too.

[sveinnfannar]

Ooops didn't mean to close/re-open 😅

What makes this tricky is that when the user passes Path or str we are responsible for opening and closing the file, while for BytesIO we get an open file handle which we then close and can't re-open.

The only solution I can think of that doesn't involve copying the whole payload into memory (which would not be great when a user passes a large BinaryIO that is backed by a file on disk) would be to read the stream in one place instead of two. That means computing size and hash only when the file is uploaded instead of when the FileUploadSpec is created.

@mwaskom can you think of a better solution?

[kramstrom]

Thanks for the test @sveinnfannar, made an implementation in this PR #1962 that seems to work

[sveinnfannar]

Great! Closing this then 👍