In open(f, ...), call flush before calling close

[Keno]

In #35217, I noticed that if we call close on an IOStream, but the device
that has the underlying file is full, we silently truncate the file without
error. To remidy that, introduce an explicit flush call before closing the
file. This should either succeed and reflect the changes on disk, or fail
and throw an appropriate error.

[StefanKarpinski]

👍

[KristofferC]

[just stating the obvious, all CI failed because of errors in package loading]

[StefanKarpinski]

While this is a good idea and should work, I fear that there are many kinds of IO objects where this will not work. I've seen it break things before. In general, I fear we seem to be in a state where the whole flush/close dance is very, very brittle.

[vtjnash]

@StefanKarpinski This change should be fine. It's just implicit-return that got broken by mistake.

[vtjnash]

julia> open("VERSION") do f; close(f); flush(f); end

julia> open(`cat`, "w") do f; close(f); flush(f); end
ERROR: IOError: stream is closed or unusable

^ Do we have any tests like these?

[StefanKarpinski]

If it works on all built-in IO types, I'll be happy (a little surprised, but happy), but I've definitely seen external IO types that I suspect would break with this change, e.g. TranscodingStreams.

[JeffBezanson]

I'm also fine with this in theory but I'm worried about breakage. Why not add a call to flush in close(::IOStream) instead?

[JeffBezanson]

Ah, in fact ios_close already calls ios_flush internally, it just doesn't report the error.

[Keno]

Ah, in fact ios_close already calls ios_flush internally, it just doesn't report the error.

Right, so the question is should close throw an error instead. And when we talked about it, the sense was that close should not error, which is why I'm proposing this instead.

The point is at some point, we should throw an error here, because otherwise subsequent code assumes that the file operation was complete, which may not be the case.

[JeffBezanson]

It seems strange to me to only throw the error if you use the open(::Function,...) form. It also conflicts with having a more general with-style construct --- is the destructor/finalizer for a file close, or flush plus close? What do other systems do in this situation?

[Keno]

fclose returns an error code, but not sure if anybody ever checks it.

[JeffBezanson]

close methods in Java and Python throw errors in this kind of situation, so I think it would be simpler and more consistent to propagate the error from the existing flush call inside close.

[StefanKarpinski]

Agree. If write stuff and close a file and it didn't get successfully written to the file, I want an error.

[JeffBezanson]

If flush fails inside close, should we continue closing the file anyway, or leave the file open? My gut reaction is that it's more predictable to close it no matter what, but that does mean you could lose buffered data.

[StefanKarpinski]

Agree: close is better. If there's an error, you can't really assume the resulting file is in any kind of recoverable state anyway.