Skip to content

Fixed a potential memory leak risk in MemoryStreamExtension.Write #562

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

Closed
Vincent-X-Zhang wants to merge 1 commit into from
Closed

Fixed a potential memory leak risk in MemoryStreamExtension.Write #562

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 9 additions & 4 deletions S7.Net/Helper/MemoryStreamExtension.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,15 @@ public static void Write(this MemoryStream stream, ReadOnlySpan<byte> value)
{
byte[] buffer = ArrayPool<byte>.Shared.Rent(value.Length);

value.CopyTo(buffer);
stream.Write(buffer, 0, value.Length);

ArrayPool<byte>.Shared.Return(buffer);
try
{
value.CopyTo(buffer);
stream.Write(buffer, 0, value.Length);
}
finally
{
ArrayPool<byte>.Shared.Return(buffer);
Copy link
Collaborator

@gfoidl gfoidl Feb 19, 2026

Choose a reason for hiding this comment

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

The try-finally is actually not what one wants here. See dotnet/runtime#48257 (reply in thread) and the following comments.

Thanks for trying to address a problem anyway!
But I'd recommend to close this PR due the reasons mentioned in the link above.

Copy link
Author

@Vincent-X-Zhang Vincent-X-Zhang Feb 24, 2026

Choose a reason for hiding this comment

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

I also recommend introducing System.IO.Pipelines to replace MemoryStream for improved performance. And what do you think?

Copy link
Collaborator

@gfoidl gfoidl Feb 24, 2026

Choose a reason for hiding this comment

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

what do you think?

I think the whole codebase is quite aged and could be put onto new feets. But this is a big effort.
Moving to Pipelines may be one step in that direction, and I would welcome such a change.

Copy link
Member

@mycroes mycroes Feb 24, 2026

Choose a reason for hiding this comment

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

In all honesty I'm not sure if it wouldn't be better to concentrate efforts on different libraries. Sally7 for instance is way more memory optimized than S7NetPlus (and used in production for over 10 years), but uses the same outdated protocol as S7NetPlus. OTOH there's new efforts like thomas-v2/S7CommPlusDriver that support newer protocols.

One of the benefits of S7NetPlus is that it allows reading classes and structs, but the implementation is quirky, has no automated tests (like most of S7NetPlus for that matter) and could generally be hugely improved. Regardless I'm not confident about the value of this part, I haven't had the need to read classes or structs personally and one could still just read byte arrays and do conversion afterwards.

If you'd still want to put in effort into S7NetPlus my suggestion would be to start creating test coverage and go from there.

Copy link
Author

@Vincent-X-Zhang Vincent-X-Zhang Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I tried to fork a repository's code and make modifications. Here’s what I plan to do:

  1. Refactor the method body without changing the API signature.
  2. Add more unit tests.
  3. I will push the code once the tests are thorough and appropriate.

Welcome to wathc my repository and provide any good suggestions. Thx! @mycroes @gfoidl

}
}
}
#endif
Expand Down