Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.NET 6 regression on writing to character device #59754

Closed
am11 opened this issue Sep 29, 2021 · 14 comments
Closed

.NET 6 regression on writing to character device #59754

am11 opened this issue Sep 29, 2021 · 14 comments

Comments

@am11
Copy link
Member

am11 commented Sep 29, 2021

Create a console app with following content in Program.cs:

using System;
using System.IO;
using System.Threading;
using System.Threading.Tasks;

string path = "/dev/tty";
if (!File.Exists(path))
{
    throw new Exception(path + " is not available in this environment.");
}

var contentBytes = new byte[] { 1, 2, 3 };

using (var cts = new CancellationTokenSource())
{
    Task writingTask = File.WriteAllBytesAsync(path, contentBytes, cts.Token);
    cts.CancelAfter(TimeSpan.FromMilliseconds(500));

    await writingTask;
}

Console.WriteLine("Passed!");

On macOS (x64), run with .NET 5 (5.0.400), it prints Passed!. In .NET 6 (6.0.100-rtm.21479.10) it throws:

Unhandled exception. System.IO.IOException: Device not configured : '/dev/tty'
   at System.IO.RandomAccess.WriteAtOffset(SafeFileHandle handle, ReadOnlySpan`1 buffer, Int64 fileOffset)
   at Microsoft.Win32.SafeHandles.SafeFileHandle.ThreadPoolValueTaskSource.ExecuteInternal()
--- End of stack trace from previous location ---
   at Microsoft.Win32.SafeHandles.SafeFileHandle.ThreadPoolValueTaskSource.GetResult(Int16 token)
   at Microsoft.Win32.SafeHandles.SafeFileHandle.ThreadPoolValueTaskSource.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token)
   at System.IO.File.<WriteAllBytesAsync>g__Core|72_0(String path, Byte[] bytes, CancellationToken cancellationToken)
   at Program.<Main>$(String[] args) in /Users/adeel/projects/testdevtty/Program.cs:line 21
   at Program.<Main>(String[] args)
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.IO untriaged New issue has not been triaged by the area owner labels Sep 29, 2021
@ghost
Copy link

ghost commented Sep 29, 2021

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

Issue Details

Create a console app with following content in Program.cs:

using System;
using System.IO;
using System.Threading;
using System.Threading.Tasks;

string path = "/dev/tty";
if (!File.Exists(path))
{
    throw new Exception(path + " is not available in this environment.");
}

var contentBytes = new byte[] { 1, 2, 3 };

using (var cts = new CancellationTokenSource())
{
    Task writingTask = File.WriteAllBytesAsync(path, contentBytes, cts.Token);
    cts.CancelAfter(TimeSpan.FromMilliseconds(500));

    await writingTask;
}

Console.WriteLine("Passed!");

On macOS (x64), run with .NET 5 (5.0.400), it prints Passed!. In .NET 6 (6.0.100-rtm.21479.10) it throws:

Unhandled exception. System.IO.IOException: Device not configured : '/dev/tty'
   at System.IO.RandomAccess.WriteAtOffset(SafeFileHandle handle, ReadOnlySpan`1 buffer, Int64 fileOffset)
   at Microsoft.Win32.SafeHandles.SafeFileHandle.ThreadPoolValueTaskSource.ExecuteInternal()
--- End of stack trace from previous location ---
   at Microsoft.Win32.SafeHandles.SafeFileHandle.ThreadPoolValueTaskSource.GetResult(Int16 token)
   at Microsoft.Win32.SafeHandles.SafeFileHandle.ThreadPoolValueTaskSource.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token)
   at System.IO.File.<WriteAllBytesAsync>g__Core|72_0(String path, Byte[] bytes, CancellationToken cancellationToken)
   at Program.<Main>$(String[] args) in /Users/adeel/projects/testdevtty/Program.cs:line 21
   at Program.<Main>(String[] args)
Author: am11
Assignees: -
Labels:

area-System.IO, untriaged

Milestone: -

@am11
Copy link
Member Author

am11 commented Sep 29, 2021

This is not reproducible on Linux with .NET 6, so macOS (and likely other non-Linux Unices) are affected.

@am11
Copy link
Member Author

am11 commented Sep 29, 2021

Output of sudo dtruss -f ~/.dotnet6/dotnet bin/Release/net6.0/testdevtty.dll 2>&1 | curl -F"sprunge=<-" http://sprunge.us: http://sprunge.us/6QWmAA

^ @tmds, open("/dev/tty" looks normal, nothing is jumping out at me. 😕

@tmds
Copy link
Member

tmds commented Sep 29, 2021

Your stacktrace is different:

Unhandled exception. System.IO.IOException: Device not configured : '/dev/tty'
   at System.IO.RandomAccess.WriteAtOffset(SafeFileHandle handle, ReadOnlySpan`1 buffer, Int64 fileOffset)
   at Microsoft.Win32.SafeHandles.SafeFileHandle.ThreadPoolValueTaskSource.ExecuteInternal()
--- End of stack trace from previous location ---
   at Microsoft.Win32.SafeHandles.SafeFileHandle.ThreadPoolValueTaskSource.GetResult(Int16 token)
   at Microsoft.Win32.SafeHandles.SafeFileHandle.ThreadPoolValueTaskSource.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token)
   at System.IO.File.<WriteAllBytesAsync>g__Core|72_0(String path, Byte[] bytes, CancellationToken cancellationToken)
   at Program.<Main>$(String[] args) in /Users/am11/projects/testdevtty/Program.cs:line 19
   at Program.<Main>(String[] args)

This is interesting:

lseek(0x14, 0x0, 0x1)		 = 0 0

.NET does this to find out whether the file is 'seekable'.

On Linux this fails with ESPIPE. On macOS it succeeds.

That determines whether bytes are written using write or pwrite (introduced in .NET 6):

int bytesWritten = handle.CanSeek ?
Interop.Sys.PWrite(handle, bufPtr, GetNumberOfBytesToWrite(buffer.Length), fileOffset) :
Interop.Sys.Write(handle, bufPtr, GetNumberOfBytesToWrite(buffer.Length));

So on Linux this uses write as before, while on macOS this is now using pwrite which fails.

@tmds
Copy link
Member

tmds commented Sep 29, 2021

Your stacktrace is different:

I mean from the one in CI on #58434 which fails on open with the same message. I assume that has to do with how the terminal is setup in CI, and is not a regression.

@stephentoub stephentoub added this to the 6.0.0 milestone Sep 29, 2021
@am11
Copy link
Member Author

am11 commented Sep 29, 2021

I mean from the one in CI on #58434 which fails on open with the same message

On macOS, I can reproduce that test failure with same stacktrace locally.

tests % pushd /Users/am11/projects/runtime/artifacts/bin/System.IO.FileSystem.Tests/net7.0-Unix-Debug
  /Users/am11/projects/runtime/artifacts/bin/testhost/net7.0-OSX-Debug-x64/dotnet exec --runtimeconfig System.IO.FileSystem.Tests.runtimeconfig.json --depsfile System.IO.FileSystem.Tests.deps.json xunit.console.dll System.IO.FileSystem.Tests.dll -xml testResults.xml -nologo -method System.IO.Tests.File_ReadWriteAllBytes.ReadAllBytes_NonSeekableFileStream_InUnix -notrait category=OuterLoop -notrait category=failing
~/projects/runtime/artifacts/bin/System.IO.FileSystem.Tests/net7.0-Unix-Debug ~/projects/runtime/src/libraries/System.IO.FileSystem/tests
  Discovering: System.IO.FileSystem.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.IO.FileSystem.Tests (found 1 of 3860 test case)
  Starting:    System.IO.FileSystem.Tests (parallel test collections = on, max threads = 12)
    System.IO.Tests.File_ReadWriteAllBytes.ReadAllBytes_NonSeekableFileStream_InUnix [FAIL]
      System.IO.IOException : Device not configured : '/dev/tty'
      Stack Trace:
        /Users/am11/projects/runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Unix.cs(24,0): at System.IO.Strategies.FileStreamHelpers.CheckFileCall(Int64 result, String path, Boolean ignoreNotSupported)
        /Users/am11/projects/runtime/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs(97,0): at System.IO.RandomAccess.WriteAtOffset(SafeFileHandle handle, ReadOnlySpan`1 buffer, Int64 fileOffset)
        /Users/am11/projects/runtime/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.ThreadPoolValueTaskSource.cs(102,0): at Microsoft.Win32.SafeHandles.SafeFileHandle.ThreadPoolValueTaskSource.ExecuteInternal()
        --- End of stack trace from previous location ---
        /Users/am11/projects/runtime/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.ThreadPoolValueTaskSource.cs(71,0): at Microsoft.Win32.SafeHandles.SafeFileHandle.ThreadPoolValueTaskSource.GetResult(Int16 token)
        /Users/am11/projects/runtime/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.ThreadPoolValueTaskSource.cs(65,0): at Microsoft.Win32.SafeHandles.SafeFileHandle.ThreadPoolValueTaskSource.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token)
        /Users/am11/projects/runtime/src/libraries/System.Private.CoreLib/src/System/IO/File.cs(848,0): at System.IO.File.<WriteAllBytesAsync>g__Core|72_0(String path, Byte[] bytes, CancellationToken cancellationToken)
        /Users/am11/projects/runtime/src/libraries/System.IO.FileSystem/tests/File/ReadWriteAllBytes.cs(228,0): at System.IO.Tests.File_ReadWriteAllBytes.ReadAllBytes_NonSeekableFileStream_InUnix()
        --- End of stack trace from previous location ---
  Finished:    System.IO.FileSystem.Tests
=== TEST EXECUTION SUMMARY ===
   System.IO.FileSystem.Tests  Total: 1, Errors: 0, Failed: 1, Skipped: 0, Time: 3.470s

I am still building PR branch on Ubuntu, but I have a theory the test is failing on Linux with ENXIO because output and error streams (not input stream) are redirected, we would probably just need to skip the test in redirect case.

@tmds
Copy link
Member

tmds commented Sep 30, 2021

The root cause is that CanSeek does not indicate support for pwrite/pread.

I see two options:

  • Let Stream use write/read always. Then using Stream always works. Using RandomAccess may not work for files that don't support random access (like /dev/tty).
    This has a behavioral impact on Stream which does now do its own position tracking and uses that offset for reading/writing.
  • Make RandomAccess fall back from pread/pwrite to read/write after observing specific errors like ESPIPE and ENXIO.

@stephentoub @am11 @jozkee @carlossanlop @adamsitnik do you prefer one of these options? or see other options?

@jozkee
Copy link
Member

jozkee commented Sep 30, 2021

Why lseek succeds in macOS? Even the doc page says that it should fail with ESPIPE for a FIFO.
Could another approach be: fix CanSeek in macOS so it can behave as Linux?

@jozkee
Copy link
Member

jozkee commented Sep 30, 2021

Let Stream use write/read always. Then using Stream always works. Using RandomAccess may not work for files that don't support random access (like /dev/tty).
This has a behavioral impact on Stream which does now do its own position tracking and uses that offset for reading/writing.

Could there be any perf. regressions in doing this? I assume perf. was the main reason why we changed to pread/pwrite.

@jozkee jozkee removed the untriaged New issue has not been triaged by the area owner label Sep 30, 2021
@am11
Copy link
Member Author

am11 commented Sep 30, 2021

Make RandomAccess fall back from pread/pwrite to read/write after observing specific errors like ESPIPE and ENXIO.

Perhaps a bit more defensive approach:
if ((e is ESPIPE || e is ENXIO) && device.mode is not S_IFREG) { /* fallback to .NET 5 behavior */ }

@tmds
Copy link
Member

tmds commented Sep 30, 2021

Why lseek succeds in macOS? Even the doc page says that it should fail with ESPIPE for a FIFO.
Could another approach be: fix CanSeek in macOS so it can behave as Linux?

/dev/tty is a character oriented device, and not a fifo.

I don't know how to fix CanSeek so it is guaranteed to match supports pread/pwrite.

I think it's better to do this based on the pread/pwrite errno, which actually saves you the cost of the syscalls performed in 'improved' CanSeek.

Could there be any perf. regressions in doing this? I assume perf. was the main reason why we changed to pread/pwrite.

I think not. It impacts the ordering of concurrent reads/writes, which is well defined when using pread/pwrite.

Personally, I think users should be using RandomAccess and be explicit about the offsets if they want to want to do concurrent operations.

Perhaps a bit more defensive approach:
if ((e is ESPIPE || e is ENXIO) && device.mode is not S_IFREG) { /* fallback to .NET 5 behavior */ }

device.mode is not S_IFREG is not needed (and would require an extra syscall).

fallback to .NET 5 behavior. That is: use read/write instead of pread/pwrite, or do you mean something else?

@stephentoub
Copy link
Member

do you prefer one of these options? or see other options?

We should do:

Make RandomAccess fall back from pread/pwrite to read/write after observing specific errors like ESPIPE and ENXIO.

We plan to allow RandomAccess to be used with arbitrary file handles, not just seekable ones, and not just unseekable ones from FileStream, e.g.
#58381
so we shouldn't further special-case FileStream with internal hooks into RandomAccess. One of the issues I raised from the get-go is RandomAccess isn't the ideal name given this, but it's what we chose and it could be worse :)

@jozkee
Copy link
Member

jozkee commented Sep 30, 2021

@stephentoub so should the plan be to merge the PR #58506 and then backport it to 6.0? If so, do you think is far from merging?

In the other hand, I would prefer @tmds's suggested fix since is minimal and less risky to backport.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 30, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 11, 2021
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Oct 12, 2021
@jozkee jozkee closed this as completed Oct 13, 2021
@am11
Copy link
Member Author

am11 commented Oct 14, 2021

Fix has landed in 6.0.100-rtm.21514.1. Thanks @jozkee! 🎉

@ghost ghost locked as resolved and limited conversation to collaborators Nov 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants