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

Use FileOptions.Asynchronous when doing async IO #2488

Merged
merged 3 commits into from
Jul 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions src/ImageSharp/IO/IFileSystem.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Six Labors.
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

namespace SixLabors.ImageSharp.IO;
Expand All @@ -9,16 +9,32 @@ namespace SixLabors.ImageSharp.IO;
internal interface IFileSystem
{
/// <summary>
/// Returns a readable stream as defined by the path.
/// Opens a file as defined by the path and returns it as a readable stream.
/// </summary>
/// <param name="path">Path to the file to open.</param>
/// <returns>A stream representing the file to open.</returns>
/// <returns>A stream representing the opened file.</returns>
Stream OpenRead(string path);
Neme12 marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Creates or opens a file and returns it as a writable stream as defined by the path.
/// Opens a file as defined by the path and returns it as a readable stream
/// that can be used for asynchronous reading.
/// </summary>
/// <param name="path">Path to the file to open.</param>
/// <returns>A stream representing the file to open.</returns>
/// <returns>A stream representing the opened file.</returns>
Stream OpenReadAsynchronous(string path);

Neme12 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I would use Async as the suffix to match convention.

Copy link
Contributor Author

@Neme12 Neme12 Jul 5, 2023

Choose a reason for hiding this comment

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

@JimBobSquarePants I agree that the Asynchronous suffix looks weird, but I didn't use the Async suffix because the method isn't actually async and doesn't return a Task or anything awaitable. Having that suffix would be misleading and would actually go against the convention. Do you really want to go against the guidelines and have an Async suffix even though the method is synchronous and can't be awaited?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, I misread the code before. Naming is fine then. I'm not that precious about it since it's internal.

/// <summary>
/// Creates or opens a file as defined by the path and returns it as a writable stream.
/// </summary>
/// <param name="path">Path to the file to open.</param>
/// <returns>A stream representing the opened file.</returns>
Stream Create(string path);

/// <summary>
/// Creates or opens a file as defined by the path and returns it as a writable stream
/// that can be used for asynchronous reading and writing.
/// </summary>
/// <param name="path">Path to the file to open.</param>
/// <returns>A stream representing the opened file.</returns>
Stream CreateAsynchronous(string path);
}
20 changes: 19 additions & 1 deletion src/ImageSharp/IO/LocalFileSystem.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Six Labors.
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

namespace SixLabors.ImageSharp.IO;
Expand All @@ -11,6 +11,24 @@ internal sealed class LocalFileSystem : IFileSystem
/// <inheritdoc/>
public Stream OpenRead(string path) => File.OpenRead(path);

/// <inheritdoc/>
public Stream OpenReadAsynchronous(string path) => File.Open(path, new FileStreamOptions
{
Mode = FileMode.Open,
Access = FileAccess.Read,
Share = FileShare.Read,
Options = FileOptions.Asynchronous,
});

/// <inheritdoc/>
public Stream Create(string path) => File.Create(path);

/// <inheritdoc/>
public Stream CreateAsynchronous(string path) => File.Open(path, new FileStreamOptions
{
Mode = FileMode.Create,
Access = FileAccess.ReadWrite,
Share = FileShare.None,
Options = FileOptions.Asynchronous,
});
}
8 changes: 4 additions & 4 deletions src/ImageSharp/Image.FromFile.cs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public static IImageFormat DetectFormat(DecoderOptions options, string path)
{
Guard.NotNull(options, nameof(options));

using Stream stream = options.Configuration.FileSystem.OpenRead(path);
await using Stream stream = options.Configuration.FileSystem.OpenReadAsynchronous(path);
return await DetectFormatAsync(options, stream, cancellationToken).ConfigureAwait(false);
}

Expand Down Expand Up @@ -144,7 +144,7 @@ public static Task<ImageInfo> IdentifyAsync(string path, CancellationToken cance
CancellationToken cancellationToken = default)
{
Guard.NotNull(options, nameof(options));
using Stream stream = options.Configuration.FileSystem.OpenRead(path);
await using Stream stream = options.Configuration.FileSystem.OpenReadAsynchronous(path);
return await IdentifyAsync(options, stream, cancellationToken).ConfigureAwait(false);
}

Expand Down Expand Up @@ -214,7 +214,7 @@ public static Task<Image> LoadAsync(string path, CancellationToken cancellationT
string path,
CancellationToken cancellationToken = default)
{
using Stream stream = options.Configuration.FileSystem.OpenRead(path);
await using Stream stream = options.Configuration.FileSystem.OpenReadAsynchronous(path);
return await LoadAsync(options, stream, cancellationToken).ConfigureAwait(false);
}

Expand Down Expand Up @@ -291,7 +291,7 @@ public static Task<Image<TPixel>> LoadAsync<TPixel>(string path, CancellationTok
Guard.NotNull(options, nameof(options));
Guard.NotNull(path, nameof(path));

using Stream stream = options.Configuration.FileSystem.OpenRead(path);
await using Stream stream = options.Configuration.FileSystem.OpenReadAsynchronous(path);
return await LoadAsync<TPixel>(options, stream, cancellationToken).ConfigureAwait(false);
}
}
2 changes: 1 addition & 1 deletion src/ImageSharp/ImageExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public static void Save(this Image source, string path, IImageEncoder encoder)
Guard.NotNull(path, nameof(path));
Guard.NotNull(encoder, nameof(encoder));

using Stream fs = source.GetConfiguration().FileSystem.Create(path);
await using Stream fs = source.GetConfiguration().FileSystem.CreateAsynchronous(path);
await source.SaveAsync(fs, encoder, cancellationToken).ConfigureAwait(false);
}

Expand Down
109 changes: 93 additions & 16 deletions tests/ImageSharp.Tests/IO/LocalFileSystemTests.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Six Labors.
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.
Neme12 marked this conversation as resolved.
Show resolved Hide resolved

using SixLabors.ImageSharp.IO;
Expand All @@ -11,36 +11,113 @@ public class LocalFileSystemTests
public void OpenRead()
{
string path = Path.GetTempFileName();
string testData = Guid.NewGuid().ToString();
File.WriteAllText(path, testData);
try
{
string testData = Guid.NewGuid().ToString();
File.WriteAllText(path, testData);

var fs = new LocalFileSystem();
LocalFileSystem fs = new();

using (var r = new StreamReader(fs.OpenRead(path)))
{
string data = r.ReadToEnd();
using (FileStream stream = (FileStream)fs.OpenRead(path))
using (StreamReader reader = new(stream))
{
Assert.False(stream.IsAsync);
Assert.True(stream.CanRead);
Assert.False(stream.CanWrite);

Assert.Equal(testData, data);
string data = reader.ReadToEnd();

Assert.Equal(testData, data);
}
}
finally
{
File.Delete(path);
}
}

File.Delete(path);
[Fact]
public async Task OpenReadAsynchronous()
{
string path = Path.GetTempFileName();
try
{
string testData = Guid.NewGuid().ToString();
File.WriteAllText(path, testData);

LocalFileSystem fs = new();

await using (FileStream stream = (FileStream)fs.OpenReadAsynchronous(path))
using (StreamReader reader = new(stream))
{
Assert.True(stream.IsAsync);
Assert.True(stream.CanRead);
Assert.False(stream.CanWrite);

string data = await reader.ReadToEndAsync();

Assert.Equal(testData, data);
}
}
finally
{
File.Delete(path);
}
}

[Fact]
public void Create()
{
string path = Path.GetTempFileName();
string testData = Guid.NewGuid().ToString();
var fs = new LocalFileSystem();
try
{
string testData = Guid.NewGuid().ToString();
LocalFileSystem fs = new();

using (FileStream stream = (FileStream)fs.Create(path))
using (StreamWriter writer = new(stream))
{
Assert.False(stream.IsAsync);
Assert.True(stream.CanRead);
Assert.True(stream.CanWrite);

using (var r = new StreamWriter(fs.Create(path)))
writer.Write(testData);
}

string data = File.ReadAllText(path);
Assert.Equal(testData, data);
}
finally
{
r.Write(testData);
File.Delete(path);
}
}

string data = File.ReadAllText(path);
Assert.Equal(testData, data);
[Fact]
public async Task CreateAsynchronous()
{
string path = Path.GetTempFileName();
try
{
string testData = Guid.NewGuid().ToString();
LocalFileSystem fs = new();

await using (FileStream stream = (FileStream)fs.CreateAsynchronous(path))
await using (StreamWriter writer = new(stream))
{
Assert.True(stream.IsAsync);
Assert.True(stream.CanRead);
Assert.True(stream.CanWrite);

await writer.WriteAsync(testData);
}

File.Delete(path);
string data = File.ReadAllText(path);
Assert.Equal(testData, data);
}
finally
{
File.Delete(path);
}
}
}
8 changes: 4 additions & 4 deletions tests/ImageSharp.Tests/Image/ImageSaveTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public ImageSaveTests()
[Fact]
public void SavePath()
{
var stream = new MemoryStream();
using MemoryStream stream = new();
this.fileSystem.Setup(x => x.Create("path.png")).Returns(stream);
this.image.Save("path.png");

Expand All @@ -54,7 +54,7 @@ public void SavePath()
[Fact]
public void SavePathWithEncoder()
{
var stream = new MemoryStream();
using MemoryStream stream = new();
this.fileSystem.Setup(x => x.Create("path.jpg")).Returns(stream);

this.image.Save("path.jpg", this.encoderNotInFormat.Object);
Expand All @@ -73,7 +73,7 @@ public void ToBase64String()
[Fact]
public void SaveStreamWithMime()
{
var stream = new MemoryStream();
using MemoryStream stream = new();
this.image.Save(stream, this.localImageFormat.Object);

this.encoder.Verify(x => x.Encode(this.image, stream));
Expand All @@ -82,7 +82,7 @@ public void SaveStreamWithMime()
[Fact]
public void SaveStreamWithEncoder()
{
var stream = new MemoryStream();
using MemoryStream stream = new();

this.image.Save(stream, this.encoderNotInFormat.Object);

Expand Down
6 changes: 6 additions & 0 deletions tests/ImageSharp.Tests/Image/ImageTests.ImageLoadTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ protected ImageLoadTestBase()
Stream StreamFactory() => this.DataStream;

this.LocalFileSystemMock.Setup(x => x.OpenRead(this.MockFilePath)).Returns(StreamFactory);
this.LocalFileSystemMock.Setup(x => x.OpenReadAsynchronous(this.MockFilePath)).Returns(StreamFactory);
this.topLevelFileSystem.AddFile(this.MockFilePath, StreamFactory);
this.LocalConfiguration.FileSystem = this.LocalFileSystemMock.Object;
this.TopLevelConfiguration.FileSystem = this.topLevelFileSystem;
Expand All @@ -132,6 +133,11 @@ public void Dispose()
// Clean up the global object;
this.localStreamReturnImageRgba32?.Dispose();
this.localStreamReturnImageAgnostic?.Dispose();

if (this.dataStreamLazy.IsValueCreated)
{
this.dataStreamLazy.Value.Dispose();
}
}

protected virtual Stream CreateStream() => this.TestFormat.CreateStream(this.Marker);
Expand Down
44 changes: 25 additions & 19 deletions tests/ImageSharp.Tests/TestFileSystem.cs
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
// Copyright (c) Six Labors.
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

#nullable enable

namespace SixLabors.ImageSharp.Tests;

/// <summary>
/// A test image file.
/// </summary>
public class TestFileSystem : ImageSharp.IO.IFileSystem
{
private readonly Dictionary<string, Func<Stream>> fileSystem = new Dictionary<string, Func<Stream>>(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<string, Func<Stream>> fileSystem = new(StringComparer.OrdinalIgnoreCase);

public void AddFile(string path, Func<Stream> data)
{
Expand All @@ -18,35 +20,39 @@ public void AddFile(string path, Func<Stream> data)
}
}

public Stream Create(string path)
public Stream Create(string path) => this.GetStream(path) ?? File.Create(path);

public Stream CreateAsynchronous(string path) => this.GetStream(path) ?? File.Open(path, new FileStreamOptions
{
// if we have injected a fake file use it instead
lock (this.fileSystem)
{
if (this.fileSystem.ContainsKey(path))
{
Stream stream = this.fileSystem[path]();
stream.Position = 0;
return stream;
}
}
Mode = FileMode.Create,
Access = FileAccess.ReadWrite,
Share = FileShare.None,
Options = FileOptions.Asynchronous,
});

return File.Create(path);
}
public Stream OpenRead(string path) => this.GetStream(path) ?? File.OpenRead(path);

public Stream OpenReadAsynchronous(string path) => this.GetStream(path) ?? File.Open(path, new FileStreamOptions
{
Mode = FileMode.Open,
Access = FileAccess.Read,
Share = FileShare.Read,
Options = FileOptions.Asynchronous,
});

public Stream OpenRead(string path)
private Stream? GetStream(string path)
{
// if we have injected a fake file use it instead
lock (this.fileSystem)
{
if (this.fileSystem.ContainsKey(path))
if (this.fileSystem.TryGetValue(path, out Func<Stream>? streamFactory))
{
Stream stream = this.fileSystem[path]();
Stream stream = streamFactory();
stream.Position = 0;
return stream;
}
}

return File.OpenRead(path);
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,9 @@ internal class SingleStreamFileSystem : IFileSystem

Stream IFileSystem.Create(string path) => this.stream;

Stream IFileSystem.CreateAsynchronous(string path) => this.stream;

Stream IFileSystem.OpenRead(string path) => this.stream;

Stream IFileSystem.OpenReadAsynchronous(string path) => this.stream;
}
Loading