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

Add FromFile to BinaryData #107231

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

manandre
Copy link
Contributor

@manandre manandre commented Sep 1, 2024

Closes #106203

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 1, 2024
Comment on lines 438 to 439
string path = Path.GetTempFileName();
File.WriteAllBytes(path, buffer);
Copy link
Member

Choose a reason for hiding this comment

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

Should temp files be cleaned up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed!
Also fixed for the existing test that I took as reference.

private static async Task<BinaryData> FromFileAsync(string path, bool async,
string? mediaType = default, CancellationToken cancellationToken = default)
{
using FileStream fileStream = File.OpenRead(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to use File.ReadAllBytes? Since it has that tries to minimize allocations of the byte array if the file-size is determinable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question!

  • The file-size optimization is also supported in the FromStream flow.

  • The ReadAllBytesAsync method is not available for the net462 and netstandard2.0 TFMs. Can we then fallback to the synchronous implementation for these TFMs?

  • The ReadAllBytes{Async} flow may be more efficient to avoid the intermediate copy buffer of the CopyTo{Async} flow.

Here would be the corresponding code:

        private static
#if NET6_0_OR_GREATER
        async
#endif
        Task<BinaryData> FromFileAsync(string path, bool async, string? mediaType = default, CancellationToken cancellationToken = default)
        {
#if NET6_0_OR_GREATER
            byte[] bytes;

            if (async)
            {
                bytes = await File.ReadAllBytesAsync(path, cancellationToken).ConfigureAwait(false);
            }
            else
            {
                bytes = File.ReadAllBytes(path);
            }

            return new BinaryData(bytes, mediaType);
#else
            byte[] bytes = File.ReadAllBytes(path);

            return Task.FromResult(new BinaryData(bytes, mediaType));
#endif
        }

Better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. Thanks for looking into it. Personally I don't think the potential benefit outweighs the added complexity, I would keep it as it is then.

private static async Task<BinaryData> FromFileAsync(string path, bool async,
string? mediaType = default, CancellationToken cancellationToken = default)
{
using FileStream fileStream = File.OpenRead(path);
Copy link
Member

Choose a reason for hiding this comment

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

The FileStream should be opened for asynchronous use when async: true. The way this is written, it'll always use synchronous I/O, which means async operations on it will end up being sync calls queued to the pool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with the FileStream constructor enabling async calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Memory community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Add FromFile method to BinaryData
5 participants