FileStream: Restore Position to its original value when SetLength fails #43863

jozkee · 2020-10-27T00:22:04Z

Set Position back to its original value when SetEndOfFile fails, this could be caused by not having enough space on disk for the new size of the file.

Note: In order to add a test that verifies this in CI frequently, I need to find a way to effectively test this without having to depend on disk space available.

stephentoub · 2020-10-27T00:56:02Z

src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Windows.cs

                SeekCore(_fileHandle, value, SeekOrigin.Begin);
            if (!Interop.Kernel32.SetEndOfFile(_fileHandle))
            {
+                SeekCore(_fileHandle, origPos, SeekOrigin.Begin);


Have you looked at the SetLengthCore implementation on Unix in FileStream.Unix.cs? Seems like we have a similar issue there.

On Unix, SetLength doesn't throw if it exceeds the available space.
However, FileStream.Write does throw and does not restore Position when that happens, which causes that the reported error occurs in a similar fashion.

I think we can fix that case (restore Position when Write fails) in this PR as well.

runtime/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs

Lines 666 to 672 in 3fca083

while (count > 0)

{

int bytesWritten = CheckFileCall(Interop.Sys.Write(_fileHandle, bufPtr + offset, count));

_filePosition += bytesWritten;

offset += bytesWritten;

count -= bytesWritten;

}

On Unix, SetLength doesn't throw if it exceeds the available space.

It does potentially throw if ftruncate fails with an error:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs

Line 413 in e5f6b44

CheckFileCall(Interop.Sys.FTruncate(_fileHandle, value));

It's not 100% clear to me what conditions could trigger that to fail, but the man page says it can fail with EIO if "An I/O error occurred while reading from or writing to a file system."

I tried to find a way to make ftruncate fail since it cannot fail with ENOSPC (as windows' SetEndOfFile similarly does) but I didn't succeed.
Obviously the common errors like EINVAL cannot occur because SetLength validates up-front the FileStream instance and the arguments.

On my effort I tried to get a EPERM error by setting ulimit -f 100 and then calling ftruncate(fileno(fd), (__off64_t)60000), but that didn't work.

I am convinced that there are ways in which the ftruncate function may fail when called from .NET, I just don't find the scenario to fix and test what you are originally asking for.

Any suggestion is appreciated.

David and I looked at this offline, we found a method called posix_fallocate which seems to do the same as ftruncate, but can return ENOSPC.

I noticed we do check if we have it in mono, but I did not find any usage instances. I found neither checks nor usage instances in libraries.

@jkotas do you have any additional insight regarding changing the ftruncate method for posix_fallocate?

I would be worried about changes in performance characteristics. posix_fallocate may be much more expensive (actually write zeros to disk) for some file systems.

I believe that the file systems do not generally guarantee that the write operation will succeed when the file is "big enough". Unix file systems tend to do this more because of different design philosophy, but it is a problem even if you are exclusively on Windows. For example, if you have a compressed file on Windows, your write operation may fail on out-of-disk space when you try to write something less compressible into the file.

what conditions could trigger that to fail, but the man page says it can fail with EIO if "An I/O error occurred while reading from or writing to a file system.

E.g. it is a network file system and you unplug the network cable?

jkotas · 2020-10-27T01:05:54Z

src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Windows.cs

            if (!Interop.Kernel32.SetEndOfFile(_fileHandle))
            {
+                SeekCore(_fileHandle, origPos, SeekOrigin.Begin);
                int errorCode = Marshal.GetLastWin32Error();


SeekCore may overwrite the last error. The last error from SetEndOfFile needs to be captured before calling SeekCore.

Yup, thanks.

Another approach could be to wrap the SetEndOfFile call in a try-finally and restore Position in the finally block as suggested in #8307 issue description, thoughts?

As we discussed offline, you can save the errorCode from SetEndOfFile, then call SeekCore afterwards.

carlossanlop

Regarding the tests - I recently had to fix a bug that only happened when copying a FileStream through the network and we run out of space in the remote location. You can add a manual test like I did here: https://github.com/dotnet/runtime/tree/master/src/libraries/System.IO.FileSystem/tests/ManualTests

carlossanlop · 2020-10-29T17:53:49Z

src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Windows.cs

            if (!Interop.Kernel32.SetEndOfFile(_fileHandle))
            {
+                SeekCore(_fileHandle, origPos, SeekOrigin.Begin);
                int errorCode = Marshal.GetLastWin32Error();


As we discussed offline, you can save the errorCode from SetEndOfFile, then call SeekCore afterwards.

jkotas · 2020-10-30T17:14:18Z

Thinking about this more: I am not sure whether this change is a good idea overall. We won't be reliably undo the whole operation when something fails. Does the best effort provide enough interesting guarantees?

jozkee · 2020-10-30T19:44:27Z

We won't be reliably undo the whole operation when something fails.

@jkotas I think the ask on #8307 is for restoring Position when the value provided in SetLength (that changes Position to the arg. passed) fails, so if you try to write to a file by first checking how much space is left, the Writing should start from where it originally was:

using FileStream fs = File.Create($"E:/some_file.txt");
bool success = false;
int size = 1000;

while (!success)
{
    try
    {
        fs.SetLength(size);

        int contentSize = size - fs.Position;
        byte[] content = GetContent(contentSize);

        fs.Write(content, 0, content.Length);
        success = true;
    }
    catch (IOException ex)
    {
        // Try with a smaller file size.
        size = (int)(size * 0.9);
    }
}

Assuming you have 900 bytes left. In the above example, the Write succeeds but an exception is thrown when fs is disposed, this is because Position will be at the end (900), since the SetLength call on the first iteration left the Position at 1000.

Am I understanding the problem wrong?
Is this an issue that we should address? If not then we can close this PR.

Also, I think that as a workaround you could save Position in a local variable and set it back after SetLength.

Does the best effort provide enough interesting guarantees?

I am not sure what do you mean by best effort.

jkotas · 2020-10-30T20:33:38Z

I am not sure what do you mean by best effort.

If we were to document the behavior with this change, it would be: "If the operation fails, the file position is unchanged except when resetting the position fails too.". This is not very useful guarantee to program against. It basically says that you cannot make assumptions about what the file position is going to be if the operation fails.

A strong guarantee would be: "If the operation fails, the file position is guaranteed to be unchanged.". We are not able to make this guarantee unfortunately.

jkotas · 2020-10-31T02:37:46Z

We may be able to fix this nicely by using SetFileInformationByHandle with FILE_END_OF_FILE_INFO. Could you please give it a try?

It should avoid the need for the seeks that are causing the problem. It should make the implementation on Unix similar to what I have done in #44097 .

jozkee · 2020-11-02T22:46:31Z

@jkotas can you please take a look at #44170? Thanks.

jozkee · 2020-11-03T07:29:02Z

Closing in favor of #44170.

Restore Position to its original when SetLength fails

908a0de

jozkee added the area-System.IO label Oct 27, 2020

jozkee added this to the 6.0.0 milestone Oct 27, 2020

jozkee requested review from carlossanlop and stephentoub October 27, 2020 00:22

jozkee self-assigned this Oct 27, 2020

jozkee changed the title ~~Restore Position to its original when SetLength fails~~ FileStream: Restore Position to its original value when SetLength fails Oct 27, 2020

stephentoub reviewed Oct 27, 2020

View reviewed changes

jkotas reviewed Oct 27, 2020

View reviewed changes

runfoapp bot mentioned this pull request Oct 27, 2020

HTTP2 test intermittently failing CI #43405

Closed

carlossanlop reviewed Oct 29, 2020

View reviewed changes

carlossanlop mentioned this pull request Oct 31, 2020

FileStream: Position may not be restored if SetLength fails dotnet/dotnet-api-docs#5066

Closed

jkotas mentioned this pull request Oct 31, 2020

Delete unnecessary seek from FileStream.SetLengthInternal on Unix #44097

Merged

jozkee mentioned this pull request Nov 2, 2020

Use SetFileInformationByHandle on FileStream.SetLength #44170

Merged

jozkee closed this Nov 3, 2020

ghost locked as resolved and limited conversation to collaborators Dec 6, 2020

jozkee deleted the fs_restore_position branch March 24, 2021 19:35

	while (count > 0)
	{
	int bytesWritten = CheckFileCall(Interop.Sys.Write(_fileHandle, bufPtr + offset, count));
	_filePosition += bytesWritten;
	offset += bytesWritten;
	count -= bytesWritten;
	}

FileStream: Restore Position to its original value when SetLength fails #43863

FileStream: Restore Position to its original value when SetLength fails #43863

Uh oh!

Conversation

jozkee commented Oct 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlossanlop left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Oct 30, 2020

Uh oh!

jozkee commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Oct 30, 2020

Uh oh!

jkotas commented Oct 31, 2020

Uh oh!

jozkee commented Nov 2, 2020

Uh oh!

jozkee commented Nov 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jozkee commented Oct 30, 2020 •

edited

Loading