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 16 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 @@ -48,7 +48,8 @@ protected override async Task HandleAsync(RecipeExecutionContext context)

if (!string.IsNullOrWhiteSpace(file.Base64))
{
stream = new MemoryStream(Convert.FromBase64String(file.Base64));
stream = MemoryStreamFactory.GetStream();
stream.Write(Str.FromBase64String(file.Base64));
}
else if (!string.IsNullOrWhiteSpace(file.SourcePath))
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Diagnostics;
using System.Text;
using System.Text.Json;
using Dapper;
using Fluid;
Expand Down Expand Up @@ -43,7 +44,10 @@ public AdminController(
[Admin("Queries/Sql/Query", "QueriesRunSql")]
public Task<IActionResult> Query(string query)
{
query = string.IsNullOrWhiteSpace(query) ? "" : System.Text.Encoding.UTF8.GetString(Convert.FromBase64String(query));
query = string.IsNullOrWhiteSpace(query)
? ""
: Encoding.UTF8.GetString(Str.FromBase64String(query));

return Query(new AdminQueryViewModel
{
DecodedQuery = query,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System.Diagnostics;
using System.Globalization;
using System.Text;
using System.Text.Encodings.Web;
using System.Text.Json;
using System.Text.Json.Nodes;
Expand Down Expand Up @@ -466,7 +467,9 @@ public async Task<IActionResult> Query(string indexName, string query)
return await Query(new AdminQueryViewModel
{
IndexName = indexName,
DecodedQuery = string.IsNullOrWhiteSpace(query) ? string.Empty : System.Text.Encoding.UTF8.GetString(Convert.FromBase64String(query))
DecodedQuery = string.IsNullOrWhiteSpace(query)
? string.Empty
: Encoding.UTF8.GetString(Str.FromBase64String(query))
});
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System.Diagnostics;
using System.Globalization;
using System.Text;
using System.Text.Encodings.Web;
using System.Text.Json;
using System.Text.Json.Nodes;
Expand Down Expand Up @@ -355,7 +356,10 @@ public async Task<ActionResult> Delete(LuceneIndexSettingsViewModel model)

public Task<IActionResult> Query(string indexName, string query)
{
query = string.IsNullOrWhiteSpace(query) ? "" : System.Text.Encoding.UTF8.GetString(Convert.FromBase64String(query));
query = string.IsNullOrWhiteSpace(query)
? ""
: Encoding.UTF8.GetString(Str.FromBase64String(query));

return Query(new AdminQueryViewModel { IndexName = indexName, DecodedQuery = query });
}

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.

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 @@ -160,7 +160,7 @@ public async Task<IActionResult> ResetPasswordPOST()

if (ModelState.IsValid)
{
var token = Encoding.UTF8.GetString(Convert.FromBase64String(model.ResetToken));
var token = Encoding.UTF8.GetString(Str.FromBase64String(model.ResetToken));

if (await _userService.ResetPasswordAsync(model.UsernameOrEmail, token, model.NewPassword, ModelState.AddModelError))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,15 @@ 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);
result.Save(w);
}

var content = Encoding.UTF8.GetString(stream.ToArray());
var content = Encoding.UTF8.GetString(stream.GetBuffer(), 0, (int)stream.Length);

return Content(content, "text/xml");
}

Expand Down
14 changes: 14 additions & 0 deletions src/OrchardCore/OrchardCore.Abstractions/MemoryStreamFactory.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
using Microsoft.IO;

namespace OrchardCore;

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

public static RecyclableMemoryStream GetStream(string tag = null)
=> _manager.GetStream(tag);

public static RecyclableMemoryStream GetStream(int requiredSize, string tag = null)
=> _manager.GetStream(tag, requiredSize);
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

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

Expand Down
34 changes: 34 additions & 0 deletions src/OrchardCore/OrchardCore.Abstractions/Str.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
namespace OrchardCore;

public static class Str
{
public static ReadOnlySpan<byte> FromBase64String(string base64)
sebastienros marked this conversation as resolved.
Show resolved Hide resolved
{
if (string.IsNullOrEmpty(base64))
{
return [];
}

var neededLength = GetByteArrayLengthFromBase64(base64);

using var memoryStream = MemoryStreamFactory.GetStream(neededLength);
var bytes = memoryStream.GetSpan(neededLength);

if (!Convert.TryFromBase64String(base64, bytes, out var _))
{
throw new FormatException("Invalid Base64 string.");
}

return bytes;
}

// The length of a Base64-encoded string corresponds to the original byte length.
// Base64 encoding converts every 3 bytes of data into 4 characters.
// To calculate the original byte length from the Base64 string, use the formula:
// (length * 3) / 4. This ensures the correct byte count by multiplying the length by 3
// before dividing by 4.
private static int GetByteArrayLengthFromBase64(string base64String)
{
return base64String.Length * 3 / 4;
}
}
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 @@ -24,7 +24,10 @@ public Task<byte[]> SerializeAsync<TDocument>(TDocument document, int compressTh
var data = JsonSerializer.SerializeToUtf8Bytes(document, _serializerOptions);
if (data.Length >= compressThreshold)
{
data = Compress(data);
var stream = MemoryStreamFactory.GetStream();
var length = Compress(data, stream);

data = stream.GetBuffer().AsSpan().Slice(0, length).ToArray();
}

return Task.FromResult(data);
Expand All @@ -33,14 +36,16 @@ public Task<byte[]> SerializeAsync<TDocument>(TDocument document, int compressTh
public Task<TDocument> DeserializeAsync<TDocument>(byte[] data)
where TDocument : class, IDocument, new()
{
TDocument document;

if (IsCompressed(data))
{
data = Decompress(data);
document = JsonSerializer.Deserialize<TDocument>(Decompress(data), _serializerOptions);
}
else
{
document = JsonSerializer.Deserialize<TDocument>(data, _serializerOptions);
}

using var ms = new MemoryStream(data);

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

return Task.FromResult(document);
}
Expand All @@ -57,37 +62,23 @@ internal static bool IsCompressed(byte[] data)
return false;
}

internal static byte[] Compress(byte[] data)
internal static int Compress(byte[] data, Stream output)
{
using var input = new MemoryStream(data);
using var output = new MemoryStream();
using (var gzip = new GZipStream(output, CompressionMode.Compress))
{
input.CopyTo(gzip);
}
using var gZip = new GZipStream(output, CompressionMode.Compress);

if (output.TryGetBuffer(out var buffer))
{
return buffer.Array;
}
input.CopyTo(gZip);

return output.ToArray();
return (int)gZip.Length;
sebastienros marked this conversation as resolved.
Show resolved Hide resolved
}

internal static byte[] Decompress(byte[] data)
internal static ReadOnlySpan<byte> Decompress(byte[] data)
sebastienros marked this conversation as resolved.
Show resolved Hide resolved
{
using var input = new MemoryStream(data);
using var output = new MemoryStream();
using (var gzip = new GZipStream(input, CompressionMode.Decompress))
{
gzip.CopyTo(output);
}

if (output.TryGetBuffer(out var buffer))
{
return buffer.Array;
}
using var output = MemoryStreamFactory.GetStream();
using var gZip = new GZipStream(input, CompressionMode.Decompress);
gZip.CopyTo(output);

return output.ToArray();
return output.GetBuffer().AsSpan().Slice(0, (int)gZip.Length);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ public class CommonGeneratorMethods : IGlobalMethodProvider
Name = "base64",
Method = serviceProvider => (Func<string, string>)(encoded =>
{
return Encoding.UTF8.GetString(Convert.FromBase64String(encoded));
return Encoding.UTF8.GetString(Str.FromBase64String(encoded));
}),
};

Expand All @@ -28,26 +28,30 @@ 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 = MemoryStreamFactory.GetStream();
sebastienros marked this conversation as resolved.
Show resolved Hide resolved
stream.Write(Str.FromBase64String(encoded));
stream.Seek(0, SeekOrigin.Begin);

var decompressed = new MemoryStream();
using var gZip = new GZipStream(stream, CompressionMode.Decompress, leaveOpen: true);

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

while ((nRead = gzip.Read(buffer, 0, buffer.Length)) > 0)
while ((nRead = gZip.Read(buffer, 0, buffer.Length)) > 0)
{
decompressed.Write(buffer, 0, nRead);
}

return Convert.ToBase64String(decompressed.ToArray());
return Convert.ToBase64String(decompressed.GetBuffer(), 0, (int)decompressed.Length);
}),
};

public IEnumerable<GlobalMethod> GetMethods() => new[] { _base64, _html, _gZip };
public IEnumerable<GlobalMethod> GetMethods()
=> new[] { _base64, _html, _gZip };
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,11 @@ 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();
memoryStream.WriteTo(fileStream);
memoryStream.Seek(0, SeekOrigin.Begin);

return Convert.ToBase64String(memoryStream.GetBuffer().AsSpan().Slice(0, (int)memoryStream.Length));
}
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
Loading
Loading