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

[API Proposal]: Expose SafeFileHandle.CanSeek #58370

Closed
adamsitnik opened this issue Aug 30, 2021 · 5 comments
Closed

[API Proposal]: Expose SafeFileHandle.CanSeek #58370

adamsitnik opened this issue Aug 30, 2021 · 5 comments
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.IO
Milestone

Comments

@adamsitnik
Copy link
Member

Background and motivation

Recently, we have introduced new RandomAccess type (#24847) which exposes some convenient methods for File IO. To use any of these methods, users need to provide a SafeFileHandle instance which can be obtained by using File.OpenHandle (also a new method introduced in .NET 6).

As of today, all these methods support only seekable files. In contrary to FileStream, SafeFileHandle does not expose CanSeek property, so the users can't easily check whether they can use the new RandomAccess type methods.

API Proposal

namespace Microsoft.Win32.SafeHandles
{
    partial class SafeFileHandle
    {
        public bool CanSeek { get; }
    }
}

API Usage

using SafeFileHandle fileHandle = File.OpenHandle(path, FileMode.Open, FileAccess.Read);
long fileLength = fileHandle.CanSeek ? RandomAccess.GetLength(fileHandle) : -1;

Risks

None. The API has been already implemented for both Windows and Unix and it's being used by FileStream, but it's internal.

@adamsitnik adamsitnik added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Aug 30, 2021
@adamsitnik adamsitnik added this to the 7.0.0 milestone Aug 30, 2021
@adamsitnik adamsitnik self-assigned this Aug 30, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.IO untriaged New issue has not been triaged by the area owner labels Aug 30, 2021
@ghost
Copy link

ghost commented Aug 30, 2021

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

Issue Details

Background and motivation

Recently, we have introduced new RandomAccess type (#24847) which exposes some convenient methods for File IO. To use any of these methods, users need to provide a SafeFileHandle instance which can be obtained by using File.OpenHandle (also a new method introduced in .NET 6).

As of today, all these methods support only seekable files. In contrary to FileStream, SafeFileHandle does not expose CanSeek property, so the users can't easily check whether they can use the new RandomAccess type methods.

API Proposal

namespace Microsoft.Win32.SafeHandles
{
    partial class SafeFileHandle
    {
        public bool CanSeek { get; }
    }
}

API Usage

using SafeFileHandle fileHandle = File.OpenHandle(path, FileMode.Open, FileAccess.Read);
long fileLength = fileHandle.CanSeek ? RandomAccess.GetLength(fileHandle) : -1;

Risks

None. The API has been already implemented for both Windows and Unix and it's being used by FileStream, but it's internal.

Author: adamsitnik
Assignees: adamsitnik
Labels:

area-System.IO, untriaged, api-ready-for-review

Milestone: 7.0.0

@adamsitnik adamsitnik removed the untriaged New issue has not been triaged by the area owner label Aug 30, 2021
@stephentoub
Copy link
Member

stephentoub commented Aug 30, 2021

As of today, all these methods support only seekable files.

Assuming in .NET 7 we remove this constraint, and RandomAccess' methods can be used with any SafeFileHandle regardless of whether it's seekable or not (and if not then the offset will just be ignored), is there still utility for exposing this property as well in .NET 7?

@adamsitnik
Copy link
Member Author

is there still utility for exposing this property as well in .NET 7?

I think that we are going to remove the constraint for all Read* and Write* methods, but keep it for GetLength (unless we want to return negative value to indicate that it's non-seekable, but it would be different than FileStream which throws for such files).

Also, there might be some value in terms of validating that given path specified by the user does not point to a regular file.

@terrajobst terrajobst added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 31, 2021
@terrajobst
Copy link
Member

@adamsitnik We took a quick look today. We think it's OK, but it would be good to get your take on our questions/feedback.

  • Unless there is a strong reason not to, it seems we'd want all three capability APIs from Stream, assuming the goal is to be determine whether the handle can be used to for a certain scenario
    • It seems there is not a lot of value in exposing CanRead and CanWrite because generally the handle that is acquired is known to be readable/writable, whereas seekability is a function of what the file is backed by (e.g. a pipe)
    • Should this be moved to RandomAccess instead?
namespace Microsoft.Win32.SafeHandles
{
    partial class SafeFileHandle
    {
        public bool CanSeek { get; }
    }
}

@tmds
Copy link
Member

tmds commented Oct 4, 2021

the users can't easily check whether they can use the new RandomAccess type methods

Through #59754, we learned that CanSeek does not indicate support for RandomAccess.

#58381 will relax RandomAccess so it becomes usable with non-seekable files.

I think this means the rationale for this API is no longer valid, and that it shouldn't be added.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.IO
Projects
None yet
Development

No branches or pull requests

4 participants