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

Support unseekable filestream when ReadAllBytes[Async] #58434

Conversation

lateapexearlyspeed
Copy link
Contributor

…dAllBytes[Async].

Fix #58383 , will update PR status when it is ready for review.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 31, 2021
@ghost
Copy link

ghost commented Aug 31, 2021

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

Issue Details

…dAllBytes[Async].

Fix #58383 , will update PR status when it is ready for review.

Author: lateapexearlyspeed
Assignees: -
Labels:

area-System.IO

Milestone: -

@danmoseley danmoseley changed the title lateapexearlyspeed-issue-58383 Support unseekable filestream when Rea… Support unseekable filestream when ReadAllBytes[Async] Sep 1, 2021
…issue-58383-SupportUnSeekableReadBytes

# Conflicts:
#	src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs
@lateapexearlyspeed lateapexearlyspeed marked this pull request as ready for review September 14, 2021 02:42
@am11
Copy link
Member

am11 commented Sep 29, 2021

It looks like there is some difference in how character devices are treated in .NET 5 vs. .NET 6.

e.g. this simple console program:

using System;
using System.IO;
using System.Threading;
using System.Threading.Tasks;
//using Xunit;

FileTests fileTests = new();
await fileTests.ReadAllBytes_NonSeekableFileStream_InUnix();
Console.WriteLine("Passed!");

public class FileTests
{
        // [Fact]
        public async Task ReadAllBytes_NonSeekableFileStream_InUnix()
        {
            var 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;
            }
        }
}

prints Passed! in .NET 5 on macOS, but with .NET 6 it throws Unhandled exception. System.IO.IOException: Device not configured : '/dev/tty'.

@tmds, @stephentoub, is this a known breaking change?

@stephentoub
Copy link
Member

is this a known breaking change?

Not to my knowledge.
cc: @jeffhandley, @adamsitnik.

@tmds
Copy link
Member

tmds commented Sep 29, 2021

prints Passed! in .NET 5 on macOS, but with .NET 6 it throws Unhandled exception. System.IO.IOException: Device not configured : '/dev/tty'.

On Linux, it prints Passed! with rc1.

From the CI stack trace it looks like the open call fails. Running the macOS equivalent of strace may reveal the difference between .NET 5 and .NET 6.

@am11 can you create an issue for this?

@am11
Copy link
Member

am11 commented Sep 29, 2021

On Linux, it prints Passed! with rc1.

I can confirm that it's macOS-only regression in .NET 6.

@am11 can you create an issue for this?

#59754

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, big thanks for your contribution @lateapexearlyspeed !

@adamsitnik
Copy link
Member

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2021

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

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2021

@adamsitnik backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: lateapexearlyspeed-issue-58383 Support unseekable filestream when ReadAllBytes[Async].
Applying: issue-58383 Refine code; Add test cases.
Applying: the docs were wrong, offsets are not ignored for non-seekable files
Using index info to reconstruct a base tree...
M	src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs
CONFLICT (content): Merge conflict in src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 the docs were wrong, offsets are not ignored for non-seekable files
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

adamsitnik added a commit to adamsitnik/runtime that referenced this pull request Nov 10, 2021
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Anipik pushed a commit that referenced this pull request Nov 12, 2021
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>

Co-authored-by: LateApexEarlySpeed <72254037+lateapexearlyspeed@users.noreply.github.com>
@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File.ReadAllBytes* should support non-seekable files
5 participants