Fallback to read/write if pread/pwrite fails #59846

jozkee · 2021-09-30T23:47:34Z

Fixes the scenario in #59754 for main (7.0).
cc @am11

ghost · 2021-09-30T23:47:44Z

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

Issue Details

Author:	Jozkee
Assignees:	Jozkee
Labels:	`area-System.IO`
Milestone:	6.0.0

jozkee · 2021-09-30T23:50:14Z

src/libraries/System.IO.FileSystem/tests/FileStream/CharacterDevice.cs

+    [PlatformSpecific(TestPlatforms.AnyUnix)]
+    public class CharacterDevice
+    {
+        const string CharacterDevicePath = "/dev/tty";


I thought the reason of getting ENXIO was because we were using a character device. I tried to repro with /dev/null and /dev/zero but I couldn't so I don't know what's special about this vs the other two character devices.

Also, this prints in console what you write to it, not sure if that may be an issue for testing.

It was the issue with /dev/tty (aka text/input environment) and /dev/console (aka physical terminal).
/dev/null has special handling https://man7.org/linux/man-pages/man4/null.4.html, I think that's why it bails early as a no-op.

src/libraries/System.IO.FileSystem/tests/FileStream/CharacterDevice.cs

am11 · 2021-10-01T00:10:36Z

src/libraries/System.IO.FileSystem/tests/FileStream/CharacterDevice.cs

+        [InlineData(FileOptions.None)]
+        [InlineData(FileOptions.Asynchronous)]


Can we pass devicePath as param and test /dev/tty, /dev/console and /dev/null? They are available in test environments AFAICT.

Done, I also included /dev/zero just in case.

stephentoub · 2021-10-01T00:33:41Z

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs

-                int result = handle.CanSeek ?
-                    Interop.Sys.PRead(handle, bufPtr, buffer.Length, fileOffset) :
-                    Interop.Sys.Read(handle, bufPtr, buffer.Length);
+                int result = Interop.Sys.PRead(handle, bufPtr, buffer.Length, fileOffset);


The issue we're facing is that CanSeek returns true but PRead fails. However, is there ever a situation where CanSeek would be false but PRead at arbitrary offsets would succeed? Seems like we should continue using Read always if CanSeek is false; otherwise we're going to be paying for a PRead that's always going to fail.

Similar question/comment for the write path.

+1

CanSeek was added to the handle pread/pwrite support. I think we should replace it by something like MaybeCanRandomAccess which optimistically defaults to true, and gets set to false in the error path.

There is an almost-approved api proposal to expose SafeFileHandle.CanSeek #58370 so I don't think we should remove it.
I added another field that caches "supports random access" and its initial value is set to CanSeek.

Seems like we should continue using Read always if CanSeek is false

Done, for read and for write as well.

There is an almost-approved api proposal to expose SafeFileHandle.CanSeek #58370 so I don't think we should remove it.

I've added a comment to that proposal: #58370 (comment).

I think we should remove CanSeek in this PR or when implementing #58381.
It makes sense to cache it as part of FileStream then.

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs

tmds · 2021-10-01T05:03:03Z

src/libraries/System.IO.FileSystem/tests/FileStream/CharacterDevice.cs

+using System.Threading.Tasks;
+using Xunit;
+
+namespace System.IO.Tests


Can this be part of the ctor_* tests that gets run from different APIs?

It was hard to figure out exacty how much is covered by those clases and I didn't want to end up with a ton of duplication (caused by the inheritance).
I would prefer to wait for @adamsitnik to come back to refactor the tests since he is more familiar with them.

src/libraries/System.IO.FileSystem/tests/FileStream/CharacterDevice.cs

* Keep avoiding pread/pwrite for unseekable files * Fallback to read/write only for known error ENXIO

* Exclude EBADF from assertion

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

tmds · 2021-10-04T06:37:04Z

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs

+                        Interop.ErrorInfo errorInfo = Interop.Sys.GetLastErrorInfo();
+                        AssertPReadPWriteKnownErrors(errorInfo.Error);
+
+                        if (errorInfo.Error == Interop.Error.ENXIO)


This needs ESPIPE also.

Can you describe in which case we can get ESPIPE?

When the path you pass in is a pipe/socket.

for example, EPIPE when using pwrite on socket:

socketpair(AF_UNIX, SOCK_STREAM, 0, [3, 4]) = 0 pwrite64(3, "hello", 5, 0) = -1 ESPIPE (Illegal seek)

CanSeek reports false for a FileStream opening a socket so in theory we will never call pwrite for that case (unless there's another distro that unexpectedly reports true).

I'm trying to add a unit test that covers this.
At least I can see that there is no issue with macOS in such case.

As part of #58381, we'll be able to remove the syscall that is made for CanSeek, which will require adding ESPIPE here.

It's safer to add it, and there is no real cost to it.

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs

src/libraries/System.IO.FileSystem/tests/FileStream/DevicesPipesAndSockets.cs

jozkee · 2021-10-07T18:08:46Z

@tmds @stephentoub @adamsitnik @carlossanlop this is ready for review.

tmds · 2021-10-08T03:41:51Z

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

+
+                return supportsRandomAccess == NullableBool.True;
+            }
+            set => _supportsRandomAccess = value ? NullableBool.True : NullableBool.False;


nit: This setter should only be used to set to false. Maybe we should add an Assert?

tmds

LGTM, thanks!

adamsitnik

The tests are excellent, the implementation is valid but I wonder if we could simplify it (please see my comment). Thank you @jozkee !

adamsitnik · 2021-10-08T07:05:45Z

src/libraries/System.IO.FileSystem/tests/FileStream/DevicesPipesAndSockets.cs

+    public class DevicesPipesAndSockets : FileSystemTest
+    {
+        [Theory]
+        [MemberData(nameof(DevicePath_FileOptions_TestData))]


just an opinion: when reviewing this file, the first thing I had to do was to scroll down and find DevicePath_FileOptions_TestData. If it was at the top, I would not need to do that.

src/libraries/System.IO.FileSystem/tests/FileStream/DevicesPipesAndSockets.cs

adamsitnik · 2021-10-08T07:21:22Z

src/libraries/System.IO.FileSystem/tests/FileStream/DevicesPipesAndSockets.cs

+            const int AF_UNIX = 1;
+            const int SOCK_STREAM = 1;
+            int* ptr = stackalloc int[2];
+            Assert.Equal(0, socketpair(AF_UNIX, SOCK_STREAM, 0, ptr));


another great idea to test non-seekable files! 👍

adamsitnik · 2021-10-08T07:27:01Z

src/libraries/System.IO.FileSystem/tests/FileStream/DevicesPipesAndSockets.cs

+        public void SocketPair_ReadWrite_Async()
+        {
+            const int AF_UNIX = 1;
+            const int SOCK_STREAM = 1;


these consts are used in more than one method, they could be declared in the type or you could move the socketpair-related logic to a separate method:

private static unsafe (FileStream read, FileStream write) CreateSocketPair() { int* ptr = stackalloc int[2]; Assert.Equal(0, socketpair(1, 1, 0, ptr)); return (new FileStream(new SafeFileHandle((IntPtr)ptr[0], ownsHandle: true), FileAccess.Read), new FileStream(new SafeFileHandle((IntPtr)ptr[1], ownsHandle: true), FileAccess.Write)); }

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs

adamsitnik

LGTM, thank you @jozkee

src/libraries/System.IO.FileSystem/tests/FileStream/DevicesPipesAndSockets.cs

…esAndSockets.cs Use Lazy for thread-safety Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>

src/libraries/System.IO.FileSystem/tests/FileStream/DevicesPipesAndSockets.cs

jozkee · 2021-10-12T02:35:35Z

/backport to release/6.0

github-actions · 2021-10-12T02:35:51Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1331368776

stephentoub · 2021-10-12T02:52:48Z

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

+            set
+            {
+                Debug.Assert(value == false); // We should only use the setter to disable random access.
+                _supportsRandomAccess = value ? NullableBool.True : NullableBool.False;


It might be worth adding a comment here about benign race conditions. It's possible two threads could interleave in such a way that one thread fails an I/O and sets _supportsRandomAccess to false and then another thread that was in the process of initializing _supportsRandomAccess overwrites it with true based on CanSeek. It's currently benign, however, because it'll get set to false again the next time I/O fails.

Fallback to read/write if pread/pwrite fails

6e028ed

jozkee added the area-System.IO label Sep 30, 2021

jozkee added this to the 6.0.0 milestone Sep 30, 2021

jozkee requested review from tmds, carlossanlop and stephentoub September 30, 2021 23:47

jozkee self-assigned this Sep 30, 2021

jozkee commented Sep 30, 2021

View reviewed changes

jozkee commented Oct 1, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/tests/FileStream/CharacterDevice.cs Outdated Show resolved Hide resolved

am11 reviewed Oct 1, 2021

View reviewed changes

stephentoub reviewed Oct 1, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs Show resolved Hide resolved

tmds reviewed Oct 1, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/tests/FileStream/CharacterDevice.cs Outdated Show resolved Hide resolved

am11 mentioned this pull request Oct 1, 2021

Support unseekable filestream when ReadAllBytes[Async] #58434

Merged

David Cantu added 2 commits October 3, 2021 17:15

* Cache whether a handle supports random access

edb79b3

* Keep avoiding pread/pwrite for unseekable files * Fallback to read/write only for known error ENXIO

Test more device paths and add tests for WriteAll*

9fb268d

jozkee requested review from stephentoub and tmds October 4, 2021 00:27

Fix CI: skip test when device can't be opened

f2e90a5

jozkee requested a review from adamsitnik October 4, 2021 04:45

* Do not use SkipTestException

8faf79b

* Exclude EBADF from assertion

tmds reviewed Oct 4, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs Outdated Show resolved Hide resolved

tmds reviewed Oct 4, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs Outdated Show resolved Hide resolved

use NullableBool in _supportsRandomAccess

7093895

runfoapp bot mentioned this pull request Oct 4, 2021

'The "Microsoft.DotNet.Compatibility.ValidatePackage" task failed unexpectedly' in libs legs #59908

Closed

jozkee added 2 commits October 5, 2021 18:43

Add tests for mkfifo and socketpair

1a76ae5

Rename test file

f358e1f

Address feedback in src

e569ecd

jozkee requested a review from tmds October 6, 2021 02:19

tmds reviewed Oct 6, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs Outdated Show resolved Hide resolved

tmds reviewed Oct 6, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs Outdated Show resolved Hide resolved

tmds reviewed Oct 6, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/tests/FileStream/DevicesPipesAndSockets.cs Outdated Show resolved Hide resolved

tmds reviewed Oct 6, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/tests/FileStream/DevicesPipesAndSockets.cs Outdated Show resolved Hide resolved

Address suggestions

7c73f83

jozkee requested a review from tmds October 7, 2021 18:07

runfoapp bot mentioned this pull request Oct 7, 2021

System.Tests.TimeZoneInfoTests.TestWindowsNlsDisplayNames is failing on rolling builds #60119

Closed

tmds reviewed Oct 8, 2021

View reviewed changes

tmds approved these changes Oct 8, 2021

View reviewed changes

adamsitnik reviewed Oct 8, 2021

View reviewed changes

adamsitnik mentioned this pull request Oct 8, 2021

Alternative solution jozkee/runtime#3

Closed

Address suggestions

565e2ab

adamsitnik approved these changes Oct 11, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/tests/FileStream/DevicesPipesAndSockets.cs Outdated Show resolved Hide resolved

Update src/libraries/System.IO.FileSystem/tests/FileStream/DevicesPip…

4378af0

…esAndSockets.cs Use Lazy for thread-safety Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>

am11 reviewed Oct 11, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/tests/FileStream/DevicesPipesAndSockets.cs Outdated Show resolved Hide resolved

Use IEnumerable<string> in Lazy and fix usages

7b79ad3

jozkee merged commit 825b264 into dotnet:main Oct 11, 2021

jozkee deleted the seekable-dev-tty branch October 11, 2021 21:18

github-actions bot mentioned this pull request Oct 12, 2021

[release/6.0] Fallback to read/write if pread/pwrite fails #60280

Merged

stephentoub reviewed Oct 12, 2021

View reviewed changes

ghost locked as resolved and limited conversation to collaborators Nov 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fallback to read/write if pread/pwrite fails #59846

Fallback to read/write if pread/pwrite fails #59846

jozkee commented Sep 30, 2021 •

edited

Loading

ghost commented Sep 30, 2021

jozkee Sep 30, 2021 •

edited

Loading

am11 Oct 1, 2021

am11 Oct 1, 2021

jozkee Oct 4, 2021

stephentoub Oct 1, 2021

tmds Oct 1, 2021

jozkee Oct 4, 2021

jozkee Oct 4, 2021

tmds Oct 4, 2021 •

edited

Loading

tmds Oct 1, 2021

jozkee Oct 4, 2021

tmds Oct 4, 2021

jozkee Oct 4, 2021

tmds Oct 4, 2021

tmds Oct 4, 2021

jozkee Oct 5, 2021 •

edited

Loading

tmds Oct 5, 2021

jozkee commented Oct 7, 2021

tmds Oct 8, 2021

tmds left a comment

adamsitnik left a comment

adamsitnik Oct 8, 2021

adamsitnik Oct 8, 2021

adamsitnik Oct 8, 2021

adamsitnik left a comment

jozkee commented Oct 12, 2021

github-actions bot commented Oct 12, 2021

stephentoub Oct 12, 2021

		[InlineData(FileOptions.None)]
		[InlineData(FileOptions.Asynchronous)]

Fallback to read/write if pread/pwrite fails #59846

Fallback to read/write if pread/pwrite fails #59846

Conversation

jozkee commented Sep 30, 2021 • edited Loading

ghost commented Sep 30, 2021

jozkee Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmds Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee commented Oct 7, 2021

Choose a reason for hiding this comment

tmds left a comment

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

jozkee commented Oct 12, 2021

github-actions bot commented Oct 12, 2021

Choose a reason for hiding this comment

jozkee commented Sep 30, 2021 •

edited

Loading

jozkee Sep 30, 2021 •

edited

Loading

tmds Oct 4, 2021 •

edited

Loading

jozkee Oct 5, 2021 •

edited

Loading