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.

Sign up for GitHub

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

Jump to bottom

Use RecyclableMemoryStream #16949

Merged
merged 23 commits into from
Nov 9, 2024
Merged

Use RecyclableMemoryStream #16949

merged 23 commits into from
Nov 9, 2024

Conversation

MikeAlhayek
Copy link
Member

Fix #16946

@sebastienros I am carious to see is RecyclableMemoryStream will have an impact on the project. If you have time to do a comparison that would be awesome.

@MikeAlhayek
Use RecyclableMemoryStream
a1b7fa9 
Fix #16946

Related Work Items: #169
@hishamco
Copy link
Member

hishamco commented Nov 4, 2024

I was waiting for Seb reply on the issue to let the author propose the PR, but seems you do it quickly :)

sebastienros
sebastienros reviewed Nov 4, 2024
View reviewed changes
Copy link
Member

@sebastienros sebastienros left a comment

Choose a reason for hiding this comment

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

I am very familiar with this API and I don't think we have many actual usages of it. At least not by replacing where we already use MemoryStream, I can explain at Tuesday's meeting.

...rchardCore.Infrastructure/Shells.Database/Configuration/DatabaseShellConfigurationSources.cs Outdated Show resolved Hide resolved
...re/OrchardCore.Infrastructure/Shells.Database/Configuration/DatabaseShellsSettingsSources.cs Outdated Show resolved Hide resolved
test/OrchardCore.Tests/Localization/PortableObjectStringLocalizerTests.cs Show resolved Hide resolved
test/OrchardCore.Tests/Modules/OrchardCore.Media/MediaEventTests.cs Show resolved Hide resolved
test/OrchardCore.Tests/Modules/OrchardCore.Media/MediaEventTests.cs Show resolved Hide resolved
src/OrchardCore/OrchardCore.Infrastructure/Scripting/Files/FilesScriptEngine.cs Outdated Show resolved Hide resolved
src/OrchardCore/OrchardCore.Infrastructure/Documents/DefaultDocumentSerializer.cs Outdated Show resolved Hide resolved
src/OrchardCore/OrchardCore.Infrastructure/Documents/DefaultDocumentSerializer.cs Outdated Show resolved Hide resolved
src/OrchardCore/OrchardCore.Infrastructure/Documents/DefaultDocumentSerializer.cs Outdated Show resolved Hide resolved
src/OrchardCore/OrchardCore.Infrastructure.Abstractions/MemoryStreamFactory.cs Outdated Show resolved Hide resolved
sebastienros
sebastienros approved these changes Nov 5, 2024
View reviewed changes
Copy link
Member

@sebastienros sebastienros left a comment

Choose a reason for hiding this comment

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

Some comments but LGTM

src/OrchardCore.Modules/OrchardCore.Media.Indexing.Pdf/Services/PdfMediaFileTextProvider.cs Outdated Show resolved Hide resolved
src/OrchardCore/OrchardCore.Infrastructure.Abstractions/MemoryStreamFactory.cs Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 7, 2024

This pull request has merge conflicts. Please resolve those before requesting a review.

MikeAlhayek
MikeAlhayek commented Nov 9, 2024
View reviewed changes
Copy link
Member Author

@MikeAlhayek MikeAlhayek left a comment

Choose a reason for hiding this comment

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

I think we should avoid these minor breaking changes. We could add the seal suggestion to #16892

src/OrchardCore/OrchardCore.Infrastructure/Scripting/CommonGeneratorMethods.cs Outdated Show resolved Hide resolved
src/OrchardCore/OrchardCore.Infrastructure/Documents/DefaultDocumentSerializer.cs Outdated Show resolved Hide resolved
@sebastienros
Copy link
Member

@MikeAlhayek reverted breaking changes

@sebastienros sebastienros merged commit 8413ef1 into main Nov 9, 2024
7 checks passed
@sebastienros sebastienros deleted the ma/RecyclableMemoryStream branch November 9, 2024 20:24
This was referenced Nov 13, 2024
Closed
Merged
@gvkries gvkries mentioned this pull request Nov 25, 2024
Merged
mvarblow
mvarblow reviewed Nov 25, 2024
View reviewed changes
Copy link
Contributor

@mvarblow mvarblow left a comment

Choose a reason for hiding this comment

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

This PR seems to have broken the database shells feature because the memory stream holding the database shells document is disposed before the configuration source (which uses the memory stream) is actually read.

...re/OrchardCore.Infrastructure/Shells.Database/Configuration/DatabaseShellsSettingsSources.cs
@@ -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.

...re/OrchardCore.Infrastructure/Shells.Database/Configuration/DatabaseShellsSettingsSources.cs
@@ -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.

src/OrchardCore.Modules/OrchardCore.Sitemaps/Controllers/SitemapController.cs
@@ -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 •
edited
Loading

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.

@MikeAlhayek MikeAlhayek mentioned this pull request Nov 26, 2024
Merged
@github-actions github-actions bot mentioned this pull request Dec 1, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvarblow mvarblow mvarblow left review comments

@sebastienros sebastienros sebastienros approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use Microsoft.IO.RecyclableMemoryStream
4 participants
@MikeAlhayek @hishamco @sebastienros @mvarblow