Fixed a potential memory leak risk in MemoryStreamExtension.Write#562
Fixed a potential memory leak risk in MemoryStreamExtension.Write#562Vincent-X-Zhang wants to merge 1 commit intoS7NetPlus:mainfrom
MemoryStreamExtension.Write#562Conversation
|
这是来自”麦冬@广西机械工业研究院”的邮箱自动回复邮件。您好，您的邮件已收到，我查阅后尽快回复您。
|
| } | ||
| finally | ||
| { | ||
| ArrayPool<byte>.Shared.Return(buffer); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I also recommend introducing System.IO.Pipelines to replace MemoryStream for improved performance. And what do you think?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I tried to fork a repository's code and make modifications. Here’s what I plan to do:
- Refactor the method body without changing the API signature.
- Add more unit tests.
- 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
4 participants
Move the function
ArrayPool<byte>.Shared.Returnto the finally block.