UploadedFile#close should leave uploaded_file in a state where #opened? is false

[jrochkind]

This means removing the reference to the now-closed (and unusable) IO object. Which meant refactoring an existing test slightly, as it tried to refer to the IO object after UploadedFile#close.

Use case where I encountered this

Normally you can call UploadedFile#download without having previously opened, it takes care of it.

tmpfile = uploaded_file.download
# it works

AND normally you can open and close an UploadedFile multiple times, no problem.

uploaded_file.open
# do things
uploaded_file.close

uploaded_file.open
# do things
uploaded_file.close
# no problem

BUT if you have first opened and closed an UploadedFile THEN try to call #download, it complains -- and it's because the UploadedFile actually still thinks it's #opened?

uploaded_file.open
uploaded_file.close

uploaded_file.opened? # => true !!!?! this seems wrong

uploaded_file.download
# RAISES `ruby/3.2.2/gems/down-5.4.1/lib/down/chunked_io.rb:157:in `readpartial': closed stream (IOError)

This is because close leaves the UploadedFile in an inconsistent state where it's underlying io is closed, BUT the UploadedFile still thinks it's opened, which #stream uses to assume the underlying IO is in fact readable.

The solution seems to be making sure #close leaves the UploadedFile in a symmetrical state as if it had never been opened in the first place -- that is, unset the @io to nil, which will make it no longer opened?

NOTE if you use #open in it's block arg form, it already was nil'ing out @io. This further supports the idea it's a bugfix to make close do so too.

shrine/lib/shrine/uploaded_file.rb

Line 102 in b586fb2

@io = nil

Backwards compat?

The fact that I also had to change that test for rake made me nervous -- is it a backwards incompat to make uploaded_file.to_io no longer refer to the closed io object after close?

But I can't think of any actual use case for this OTHER than a test like this -- why would you ever want to_io to return a closed unusable IO object? I think this was a bug all along, and is a bug fix, and anything relying on the previous behavior was relying on buggy behavior -- but it's very unlikely anything was, I can't think of any use case for wanting a closed unusable IO object back from to_io!

[janko]

The intention behind not resetting the uploaded_file once it's closed was to match the behavior of other IO objects. When working with a File object, for example, you cannot do anything anymore with it once it's closed.

The #opened? method was deliberately called this and not #open?, because it was supposed to return whether the uploaded file object had been opened in the past, not whether it's currently open.

I wanted to have this behavior to prevent unintentionally opening the same uploaded file multiple times, which is costly. If developers want to do this, they have to be explicit about it.

[jrochkind]

OK, interesting, thanks.

But you currently can call open and close multiple times, and it actually does work. This works fine:

uploaded_file.open
# do things
uploaded_file.close

uploaded_file.open
# do things
uploaded_file.close
# no problem

Is this a mistake, are you not meant to be able to do this? What about open with block argument, are you meant to be able to do that more than once?

It seems odd to me that you can do this, and you can call download by itself multiple times (this is intentional, yes?) -- but you can't call download after you've done an open/close. That seems inconsistent to me.

Calling download multiple times -- which works, and I think ought to work, does result in multiple open/close beneath the hood, I think?

It calls stream:

shrine/lib/shrine/uploaded_file.rb

Line 123 in bf43c6d

stream(tempfile, **options)

Which calls open with block form:

shrine/lib/shrine/uploaded_file.rb

Line 147 in bf43c6d

open(**options) { |io| IO.copy_stream(io, destination) }

So opening more than once is not an accident, it's meant to be supported, I think?

Is it intentional that you can call download more than once (with implicit open/close), and you can call open/close pairs more than once, but you can't call can open/close pair followed by a download without raising a closed stream error?

If developers want to do this, they have to be explicit about it.

I am not following this. Can you suggest a code example of a way to do this intentionally while being explicit about it?

[janko]

Yes, it's intentional that you can call open/close and download multiple times, because there you're more explicit that you're re-opening the file, and "download" implies network requests as well.

Here is a code example that I would consider to be implicit re-opening:

uploaded_file.open
# do things
uploaded_file.close

uploaded_file.read

Here I want an exception to be raised, to alert the developer that they probably didn't want to close the file before, and find ways to reuse the already downloaded content that Down::ChunkedIO has in its internal tempfile. If this implicitly re-opened, I can imagine the developer only noticing this code is inefficient on production, so I want to make it more difficult to make this mistake.

[jrochkind]

Thanks. I'm having trouble following, but specifically, is it intentional that after calling open/close pair you can't call download, or is that an unintentional side-effect of what you is intentional?

You can call open in block form followed by download, but you can't call open/close pair followed by download. This is what I am finding surprising and to me inconsistent.

If I want to call open/close pair followed by download, I guess i have to just explicitly open again a second time? Like after I've called open/close (not block form once), I have to keep doing it every time, is that what's intentional?

# part A
uploaded_file.open
# do things
uploaded_file.close

# part B
# I think this will work?
uploaded_file.open
tmpfile = uploaded_file.download
uploaded_file.close

While this seems relatively straightforward when written like this, it can be entirely different code sites (even in different modules/libraries/gems), that do part A and part B. But I guess you do have access to opened?... so I guess if you are ever doing a download where you can't be sure what's happened to the uploaded_file previously, this is the safe way to do it?

if uploaded_file.opened?
  uploaded_file.open
  uploaded_file.download
  uploaded_file.close
else
  uploaded_file.download
end

Wait, that's no good either, becuase opened? doens't tell you if the file is still open (in which case we should not re-open or especially close it locally) or has been previously previously opened (in which case we do need to re-open it, and clean up after ourselves by closing it).

Am I making any sense, do you see my point? Or am I misunderstanding? To me it seems a problem that if I don't know what's happened to the file before it arrived at my call site, I can't know how to safely download it.

But okay, I guess maybe you're saying that you basically shouldn't write code like that, it's meant to be infeasible to (eg) download without knowing what happened to the uploaded_file preivously, you mean to force it to be coordinated thorughout the use of an uploaded_file instead of it being possible to write decoupled code?

[janko]

I like the rationale, it seems this will make working with Shrine::UploadedFile more convenient and lead to less surprises 👍🏻

[jrochkind]

Thanks!