Close the file if TransferOpen fails.

[marshallbrekka]

Fixes #69

[fclairamb]

It's actually a bit more complex than that, see #66.

For reading, it's not an issue but for writing it means we might have failed to flush the file, which can be a core part of writing when you're using third-party online services. And this library was built with that in mind.

[marshallbrekka]

Sorry I got the impression from the issue comment that this was the route you wanted to take.

We use this at my company to provide an FTP interface to S3, I discovered this issue because we had a client that was creating bad active transfer requests.
This resulted in many calls to the s3 uploader client, which will essentially stay open forever until the "body" returns an EOF on Read.

Similar to #66, the existing FileStream interface doesn't give us any method to propagate a client error to the backing storage system. So in this case the only option was to close the file.

While its not a standard io interface, if FileStream supported the CloseWithError methods that the PipeReader and PipeWriter have, then that would allow propegation of errors to the respective parties (readers or writers).

[fclairamb]

Yes sorry, I wasn't clear. This is the direction I wanted to take but I also wanted to push it a bit further, which is to check if anything goes well.

You're interested in avoiding file descriptor. And it's equally important to make sure close/flush errors are correctly reported, these two subjects are basically handled at the same time.

I might be missing something but I don't think we need the CloseWithError here:

• If the storage fails on read/write or close: We'll detect it during io.Copy or Close()
• if the client wants to close: We'll detect it during the io.Copy and we'll close the file

Whatever happens, if we open the file, we will close it.

[fclairamb]

@marshallbrekka Ok to close it and review #71 instead?

[marshallbrekka]

I might be missing something but I don't think we need the CloseWithError here:

• If the storage fails on read/write or close: We'll detect it during io.Copy or Close()
• if the client wants to close: We'll detect it during the io.Copy and we'll close the file

That would solve the immediate issue, but taking it further I was thinking about when a client does not exit gracefully.
For example, if the connection is dropped, if the only option is calling Close, then the storage layer has no option but to commit a partial file.

If the interface was expanded to allow closing with error conditions, then the storage layer can discard partial files (if it so desires).

If there is concern about forcing all implementations to support such a mechanism, it could be declared under a 2nd interface.

Maybe something like

type FileStreamErrorCloser interface {
  CloseWithError(error) error
}

file, err := c.openFile(c.absPath(c.param), append)
tr, err := c.TransferOpen();

if err != nil {
  if closer, ok := file.(FileStreamErrorCloser); ok {
    closer.CloseWithError(err)
  } else {
    file.Close()
  } 
}

[marshallbrekka]

@fclairamb yeah that PR lgtm, would still like some more discussion around handling of failures on write. Its not terribly critical to us, but would be nice if we can omit writing partial files in cases of failure.

[fclairamb]

Oh right. Completely missed the point here.