RandomAccess.Write methods should perform full writes

[stephentoub]

Every use we have of RandomAccess.Write{Async} now is incorrect, as they all assume the methods will fully write all the data that was supplied. This highlights how error-prone the current design is, with Write not guaranteeing this and instead returning the number of bytes written. We've trained developers with every other Write method in .NET that such methods write all the supplied data (in contrast to Read methods which may not fill the supplied buffer).

We should change the RandomAccess.Write{Async} methods to return void/ValueTask.

If we don't do that, every use of RandomAccess.Write{Async} needs to be fixed, and an analyzer needs to be written that warns when the result isn't consumed.

cc: @adamsitnik, @jeffhandley

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Every use we have of RandomAccess.Write{Async} now is incorrect, as they all assume the methods will fully write all the data that was supplied. This highlights how error-prone the current design is, with Write not guaranteeing this and instead returning the number of bytes written. We've trained developers with every other Write method in .NET that such methods write all the supplied data (in contrast to Read methods which may not fill the supplied buffer).

We should change the RandomAccess.Write{Async} methods to return void/ValueTask.

If we don't do that, every use of RandomAccess.Write{Async} needs to be fixed, and an analyzer needs to be written that warns when the result isn't consumed.

cc: @adamsitnik, @jeffhandley

Author:	stephentoub
Assignees:	-
Labels:	area-System.IO, bug
Milestone:	6.0.0

[adamsitnik]

I agree that we are not always correctly handling partial writes and this should be fixed.

But the new APIs are not the source of the problem. These methods are just tiny wrappers around syscalls, which on every OS return the number of bytes transffered.
Their design is very simple and not opinionated.

We just happen to ignore their results (and this should be fixed) and in some cases, we have been doing that forever.
A good example of that is FileStream.WriteAsync implementation on Windows, which uses a WriteFile method that returns the number of bytes written, but we never checked the result and were always casting Task<int> to Task:

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

Line 1054 in 84228b7

private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory<byte> source, CancellationToken cancellationToken)

Usually, partial writes happen when the disk becomes full. So if there are X bytes of free space available and we try to write Y bytes where X < Y, the first write is going to succeed and return X. If we try to write the remaining Y-X bytes, it's going to fail.

In the case of FileStream, we have the state (Position) which we can easily update after the first write and then just throw on the second write.

In the case of RandomAccess.Write we don't have any state. How can we express the fact that we have transferred some data but also failed on the retry? And it's not going to be atomic. Please keep in mind that thread-safety was something that we wanted to have for RandomAccess so people stop misusing FileStream to start multiple concurrent file IO operations.

If we don't do that, every use of RandomAccess.Write{Async} needs to be fixed, and an analyzer needs to be written that warns when the result isn't consumed.

This would be my preferred solution. It should be relatively easy for UnixFileStreamStrategy and SyncWindowsFileStreamStrategy. For AsyncWindowsFileStreamStrategy it won't be trivial to keep it zero-alloc.

We also need to add tests that are going to exercise such scenarios (#54495).

[stephentoub]

Nothing about the API guarantees atomicity, nor can it. I don't know why we'd force every consumer of the API to need to deal with this when it could be done by the API itself. Best case, every developer does it correctly, having written lots more complicated code and has to worry about whether they got it right. Worst case, folks don't, and we've created a pit of failure that even ourselves obviously can't do correctly because call sites that both you and I have written are all wrong.

[stephentoub]

Please keep in mind that thread-safety was something that we wanted to have for RandomAccess so people stop misusing FileStream to start multiple concurrent file IO operations

I don't see how this affects that. If everything is position based, then someone doing concurrent operations needs to manually partition up the space being read/written. Whether the bytes for an individual read/write are performed atomically or not doesn't matter, as everything is done to the exact position specified. If a write we issue for 5 bytes at position 100 comes back as having written only 2 bytes, then we issue another write for 3 bytes at position 102.

[stephentoub]

If we try to write the remaining Y-X bytes, it's going to fail.

And what do you expect folks outside of the library to do differently? What is the correct way to consume these APIs, how are other developers going to do it any more efficiently than we do, and why are we pushing this pain to everyone else?

[adamsitnik]

@stephentoub After thinking about it for a while, I agree with you that we should most probably change this API to return void|ValueTask.

The problem is that I am currently on vacations and I can't provide a proper fix. I've opened #55474 with a short-term fix.
I'll be back in a week (19th of July) and then I'll be able to refactor RandomAccess API's and it's usages.

@stephentoub @jeffhandley I leave it up to you whether you want to take the short-term fix right now, wait for me until I get back or ask someone else to fix it when I am gone.

[stephentoub]

I leave it up to you whether you want to take the short-term fix right now, wait for me until I get back or ask someone else to fix it when I am gone.

I'll take care of it.