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

Async IO methods should use FileOptions.Asynchronous #2487

Closed
4 tasks done
Neme12 opened this issue Jun 29, 2023 · 2 comments · Fixed by #2488
Closed
4 tasks done

Async IO methods should use FileOptions.Asynchronous #2487

Neme12 opened this issue Jun 29, 2023 · 2 comments · Fixed by #2488

Comments

@Neme12
Copy link
Contributor

Neme12 commented Jun 29, 2023

Prerequisites

  • I have written a descriptive issue title
  • I have verified that I am running the latest version of ImageSharp
  • I have verified if the problem exist in both DEBUG and RELEASE mode
  • I have searched open and closed issues to ensure it has not already been reported

ImageSharp version

3.0.1

Environment (Operating system, version and so on)

Windows 11 Version 22H2

.NET Framework version

.NET 7.0.304

Description

Async methods that read/write from the file system, such as Image.LoadAsync and Image.SaveAsync, use async methods on FileStream but don't instantiate the FileStream with FileOptions.Asynchronous, as should be done when using async operations. This means that the implementation isn't actually truly async and instead uses synchronous IO on a new thread, needlessly using more resources.

There is a simple workaround for this by creating the FileStream manually and giving ImageSharp the stream as opposed to the file path, e.g.:

await using (var fileStream = new FileStream(path, new FileStreamOptions
{
    Mode = FileMode.Open,
    Access = FileAccess.Read,
    Share = FileShare.Read,
    Options = FileOptions.Asynchronous,
}))
using (var image = await Image.LoadAsync(fileStream))
{
// ...
}

but I feel like this is a bug and should be fixed in ImageSharp itself. The method is misleading, suggesting that it uses async IO while actually doesn't.

@antonfirsov
Copy link
Member

antonfirsov commented Jun 30, 2023

This makes sense, here is what BCL is doing for File.ReadAllTextAsync:

https://github.com/dotnet/runtime/blob/2f208128f257b2390a8edefa0a19e0eaad6bd419/src/libraries/System.Private.CoreLib/src/System/IO/File.cs#L853-L856

I guess SequentialScan would make things worse in our case, because BufferedReadStream does Seek a lot.

@Neme12 interested creating a PR?

@Neme12
Copy link
Contributor Author

Neme12 commented Jun 30, 2023

Sure, I can give it a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants