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 RecyclableMemoryStream #16949

Merged
merged 23 commits into from
Nov 9, 2024
Merged
Show file tree
Hide file tree
Changes from 9 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
1 change: 1 addition & 0 deletions Directory.Packages.props
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
See https://github.com/OrchardCMS/OrchardCore/pull/16057 for more information.
-->
<PackageVersion Include="Microsoft.IdentityModel.Protocols.OpenIdConnect" Version="8.1.0" />
<PackageVersion Include="Microsoft.IO.RecyclableMemoryStream" Version="3.0.1" />
<PackageVersion Include="Microsoft.NET.Test.Sdk" Version="17.11.1" />
<PackageVersion Include="MimeKit" Version="4.8.0" />
<PackageVersion Include="MiniProfiler.AspNetCore.Mvc" Version="4.3.8" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

<ItemGroup>
<ProjectReference Include="..\..\OrchardCore\OrchardCore.Indexing.Abstractions\OrchardCore.Indexing.Abstractions.csproj" />
<ProjectReference Include="..\..\OrchardCore\OrchardCore.Infrastructure.Abstractions\OrchardCore.Infrastructure.Abstractions.csproj" />
<ProjectReference Include="..\..\OrchardCore\OrchardCore.Media.Abstractions\OrchardCore.Media.Abstractions.csproj" />
<ProjectReference Include="..\..\OrchardCore\OrchardCore.Module.Targets\OrchardCore.Module.Targets.csproj" />
<ProjectReference Include="..\..\OrchardCore\OrchardCore.ResourceManagement\OrchardCore.ResourceManagement.csproj" />
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Cysharp.Text;
using Microsoft.IO;
using UglyToad.PdfPig;

namespace OrchardCore.Media.Indexing;
Expand All @@ -11,14 +12,14 @@ public async Task<string> GetTextAsync(string path, Stream fileStream)
// https://github.com/UglyToad/PdfPig/blob/master/src/UglyToad.PdfPig.Core/StreamInputBytes.cs#L45.
// Thus if it isn't, which is the case with e.g. Azure Blob Storage, we need to copy it to a new, seekable
// Stream.
MemoryStream seekableStream = null;
RecyclableMemoryStream seekableStream = null;
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved
try
{
if (!fileStream.CanSeek)
{
// Since fileStream.Length might not be supported either, we can't preconfigure the capacity of the
// MemoryStream.
seekableStream = new MemoryStream();
seekableStream = MemoryStreamFactory.GetStream();
// While this involves loading the file into memory, we don't really have a choice.
await fileStream.CopyToAsync(seekableStream);
seekableStream.Position = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public async Task<IActionResult> Index(string sitemapId, CancellationToken cance

document.Declaration = new XDeclaration("1.0", "utf-8", null);

var stream = new MemoryStream();
using var stream = MemoryStreamFactory.GetStream();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not using the Site Map feature, so I can't say whether this is actually a problem, but this looks wrong. With this change, the stream is disposed as it's being returned for use later (by the File result).

Copy link
Member Author

@MikeAlhayek MikeAlhayek Nov 25, 2024

Choose a reason for hiding this comment

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

@mvarblow it look suspicious to me too. Feel free to test it out, and submit an issue and PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is indeed a problem. I am fixing it.

await document.SaveAsync(stream, SaveOptions.None, cancellationToken);

if (stream.Length >= ErrorLength)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public async Task<IActionResult> ServiceEndpoint([ModelBinder(BinderType = typeo
};

// Save to an intermediate MemoryStream to preserve the encoding declaration.
using var stream = new MemoryStream();
using var stream = MemoryStreamFactory.GetStream();
using (var w = XmlWriter.Create(stream, settings))
{
var result = _writer.MapMethodResponse(methodResponse);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\..\OrchardCore\OrchardCore.Infrastructure.Abstractions\OrchardCore.Infrastructure.Abstractions.csproj" />
<ProjectReference Include="..\..\OrchardCore\OrchardCore.Module.Targets\OrchardCore.Module.Targets.csproj" />
<ProjectReference Include="..\..\OrchardCore\OrchardCore.XmlRpc.Abstractions\OrchardCore.XmlRpc.Abstractions.csproj" />
</ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,17 @@ public class BlobFileStore : IFileStore
private readonly IClock _clock;
private readonly BlobContainerClient _blobContainer;
private readonly IContentTypeProvider _contentTypeProvider;

private readonly string _basePrefix;

public BlobFileStore(BlobStorageOptions options, IClock clock, IContentTypeProvider contentTypeProvider)
public BlobFileStore(
BlobStorageOptions options,
IClock clock,
IContentTypeProvider contentTypeProvider)
{
_options = options;
_clock = clock;
_contentTypeProvider = contentTypeProvider;

_blobContainer = new BlobContainerClient(_options.ConnectionString, _options.ContainerName);

if (!string.IsNullOrEmpty(_options.BasePath))
Expand Down Expand Up @@ -436,6 +439,7 @@ private async Task CreateDirectoryAsync(string path)

// Create a directory marker file to make this directory appear when listing directories.
using var stream = new MemoryStream(MarkerFileContent);

await placeholderBlob.UploadAsync(stream);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
<ItemGroup>
<ProjectReference Include="..\OrchardCore.Abstractions\OrchardCore.Abstractions.csproj" />
<ProjectReference Include="..\OrchardCore.FileStorage.Abstractions\OrchardCore.FileStorage.Abstractions.csproj" />
<ProjectReference Include="..\OrchardCore.Infrastructure.Abstractions\OrchardCore.Infrastructure.Abstractions.csproj" />
<ProjectReference Include="..\OrchardCore.Liquid.Abstractions\OrchardCore.Liquid.Abstractions.csproj" />
</ItemGroup>

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
using Microsoft.IO;
sebastienros marked this conversation as resolved.
Show resolved Hide resolved

namespace OrchardCore;

public static class MemoryStreamFactory
{
private static readonly RecyclableMemoryStreamManager _manager = new();

public static RecyclableMemoryStream GetStream(string tag = null)
=> _manager.GetStream(tag);
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
<FrameworkReference Include="Microsoft.AspNetCore.App" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.IO.RecyclableMemoryStream" />
</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\OrchardCore.Abstractions\OrchardCore.Abstractions.csproj" />
<ProjectReference Include="..\OrchardCore.Data.Abstractions\OrchardCore.Data.Abstractions.csproj" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,7 @@ public Task<TDocument> DeserializeAsync<TDocument>(byte[] data)
data = Decompress(data);
}

using var ms = new MemoryStream(data);

var document = JsonSerializer.Deserialize<TDocument>(ms, _serializerOptions);
var document = JsonSerializer.Deserialize<TDocument>(data, _serializerOptions);

return Task.FromResult(document);
}
Expand All @@ -60,7 +58,7 @@ internal static bool IsCompressed(byte[] data)
internal static byte[] Compress(byte[] data)
{
using var input = new MemoryStream(data);
using var output = new MemoryStream();
using var output = MemoryStreamFactory.GetStream();
using (var gzip = new GZipStream(output, CompressionMode.Compress))
{
input.CopyTo(gzip);
Expand All @@ -77,7 +75,7 @@ internal static byte[] Compress(byte[] data)
internal static byte[] Decompress(byte[] data)
{
using var input = new MemoryStream(data);
using var output = new MemoryStream();
using var output = MemoryStreamFactory.GetStream();
using (var gzip = new GZipStream(input, CompressionMode.Decompress))
{
gzip.CopyTo(output);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,16 @@ public class CommonGeneratorMethods : IGlobalMethodProvider
/// Converts a Base64 encoded gzip stream to an uncompressed Base64 string.
/// See http://www.txtwizard.net/compression.
/// </summary>
private static readonly GlobalMethod _gZip = new()
private readonly GlobalMethod _gZip = new()
{
Name = "gzip",
Method = serviceProvider => (Func<string, string>)(encoded =>
{
var bytes = Convert.FromBase64String(encoded);
using var gzip = new GZipStream(new MemoryStream(bytes), CompressionMode.Decompress);
using var stream = new MemoryStream(bytes);
using var gzip = new GZipStream(stream, CompressionMode.Decompress);

var decompressed = new MemoryStream();
using var decompressed = MemoryStreamFactory.GetStream();
var buffer = new byte[1024];
int nRead;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ public object Evaluate(IScriptingScope scope, string script)
}

using var fileStream = fileInfo.CreateReadStream();
using var ms = new MemoryStream();
fileStream.CopyTo(ms);
return Convert.ToBase64String(ms.ToArray());
using var memoryStream = MemoryStreamFactory.GetStream();

return Convert.ToBase64String(memoryStream.GetBuffer());
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ public async Task AddSourcesAsync(string tenant, IConfigurationBuilder builder)
if (configuration is not null)
{
var configurationString = configuration.ToJsonString(JOptions.Default);
builder.AddTenantJsonStream(new MemoryStream(Encoding.UTF8.GetBytes(configurationString)));
using var stream = new MemoryStream(Encoding.UTF8.GetBytes(configurationString));

builder.AddTenantJsonStream(stream);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ public async Task AddSourcesAsync(IConfigurationBuilder builder)
if (document.ShellsSettings is not null)
{
var shellsSettingsString = document.ShellsSettings.ToJsonString(JOptions.Default);
builder.AddTenantJsonStream(new MemoryStream(Encoding.UTF8.GetBytes(shellsSettingsString)));
using var stream = new MemoryStream(Encoding.UTF8.GetBytes(shellsSettingsString));
Copy link
Contributor

Choose a reason for hiding this comment

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

I just updated to OC 2.1 and my (test) site no longer starts. It looks like the problem is this new "using" which results in the stream being disposed when this method exits. The problem is that the stream is added as a configuration source here, but the stream (attached to the configuration source) isn't read until later.

You can see this problem if you enable the database shells feature with OC 2.1. Note that disposing a MemoryStream isn't actually necessary. The Dispose method in MemoryStream does not release any memory, it just marks the stream as closed. So there was no harm in the old code which did not Dispose the stream.

I'll create a separate issue, but I thought it might help to add a comment here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like I wasn't the only one to hit this and it's already been addressed in #17054

Copy link
Member Author

Choose a reason for hiding this comment

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

@mvarblow we release 2.1.1 today with this fix. Please try to upgrade your project and see if the problem is fixed. If not, please open a new issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for releasing the fix so quickly! Yes, that did resolve the issue for me, though I'm now running into another blocking issue with the user timezone service. I'll write up an issue for that one.


builder.AddTenantJsonStream(stream);
}
}

Expand All @@ -53,7 +55,9 @@ public async Task AddSourcesAsync(string tenant, IConfigurationBuilder builder)
{
var shellSettings = new JsonObject { [tenant] = document.ShellsSettings[tenant] };
var shellSettingsString = shellSettings.ToJsonString(JOptions.Default);
builder.AddTenantJsonStream(new MemoryStream(Encoding.UTF8.GetBytes(shellSettingsString)));
using var stream = new MemoryStream(Encoding.UTF8.GetBytes(shellSettingsString));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same problem here.


builder.AddTenantJsonStream(stream);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,12 @@ public async Task SaveAsync(string tenant, IDictionary<string, string> data)

public async Task RemoveAsync(string tenant)
{
var appsettings = IFileStoreExtensions.Combine(null, _container, tenant, OrchardCoreConstants.Configuration.ApplicationSettingsFileName);
var appSettings = IFileStoreExtensions.Combine(null, _container, tenant, OrchardCoreConstants.Configuration.ApplicationSettingsFileName);

var fileInfo = await _shellsFileStore.GetFileInfoAsync(appsettings);
var fileInfo = await _shellsFileStore.GetFileInfoAsync(appSettings);
if (fileInfo != null)
{
await _shellsFileStore.RemoveFileAsync(appsettings);
await _shellsFileStore.RemoveFileAsync(appSettings);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
<ItemGroup>
<ProjectReference Include="..\OrchardCore.Abstractions\OrchardCore.Abstractions.csproj" />
<ProjectReference Include="..\OrchardCore.FileStorage.AzureBlob\OrchardCore.FileStorage.AzureBlob.csproj" />
<ProjectReference Include="..\OrchardCore.Infrastructure.Abstractions\OrchardCore.Infrastructure.Abstractions.csproj" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public static JsonObject GetContentStepRecipe(ContentItem contentItem, Action<Js

public async Task<HttpResponseMessage> PostRecipeAsync(JsonObject recipe, bool ensureSuccess = true)
{
using var zipStream = new MemoryStream();
await using var zipStream = MemoryStreamFactory.GetStream();
using (var zip = new ZipArchive(zipStream, ZipArchiveMode.Create, true))
{
var entry = zip.CreateEntry("Recipe.json");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ public void HtmlLocalizerDoesNotFormatTwiceIfFormattedTranslationContainsCurlyBr
{
var htmlLocalizer = new PortableObjectHtmlLocalizer(localizer);
var unformatted = htmlLocalizer["The page (ID:{0}) was deleted.", "{1}"];
var memStream = new MemoryStream();
var memStream = MemoryStreamFactory.GetStream();
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved
var textWriter = new StreamWriter(memStream);
var textReader = new StreamReader(memStream);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public async Task DisposesMediaCreatingStreams()
#pragma warning restore CA1859
try
{
inputStream = new MemoryStream();
inputStream = MemoryStreamFactory.GetStream();
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved
originalStream = inputStream;

// Add original stream to streams to maintain reference to test disposal.
Expand Down Expand Up @@ -78,7 +78,7 @@ public class TestMediaEventHandler : IMediaCreatingEventHandler
{
public async Task<Stream> MediaCreatingAsync(MediaCreatingContext context, Stream inputStream)
{
var outStream = new MemoryStream();
var outStream = MemoryStreamFactory.GetStream();
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved
await inputStream.CopyToAsync(outStream);

return outStream;
Expand Down
2 changes: 1 addition & 1 deletion test/OrchardCore.Tests/Stubs/MemoryFileBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ public class MemoryFileBuilder

public async Task SetFileAsync(string subpath, Stream stream)
{
using var ms = new MemoryStream();
using var ms = MemoryStreamFactory.GetStream();
await stream.CopyToAsync(ms);
VirtualFiles[subpath] = ms.ToArray();
}
Expand Down