Memory leak in Tag.InitializeAsync in case of exceptions #445
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was able to reproduce the memory leak with a simple program with the following structure:
If the PLC is online and the tag is readable, there is no memory leak. If there are exceptions on
ReadAsync()the process slowly increase CPU and memory usage (it may need to keep failing overnight to reach 100% CPU usage in my environment).I think the handle created by
plc_tag_create_exshould be destroyed inInitializeAsyncregardless of the outcome of the async initialization. In the current code, if the result of the initialization is notStatus.Ok, the handle is not destroyed immediately and_isInitializedis never set to true so that also theDisposemethod cannot destroy the handle.Some notes on my proposed fix:
Status.Ok) and in case of timeout; this let the caller useReadAsyncmultiple times on the sameTaginstance even in case of Exceptions and without explicitly callingInitializeAsync(since this is permitted by the API, this should also work correcly)plc_tag_create_exis moved before timeout using block to make sure that timeout timer starts only if the async operation actually starts and the handle has been createdThe new implementation is tested and it seems to properly fix the issue. Please feel free to improve the code and the its style :)