Skip to content

Conversation

@timyhac
Copy link
Collaborator

@timyhac timyhac commented Jan 25, 2025

No description provided.

var result = (Status)_native.plc_tag_destroy(nativeTagHandle);
ThrowIfStatusNotOk(result);
{
RemoveCallback();
Copy link
Collaborator Author

@timyhac timyhac Jan 25, 2025

Choose a reason for hiding this comment

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

@kyle-github - this is the method that is used to tear down the tag either explicitly or implicitly by the garbage collector.

Previously, this code was checking that neither removing the event callback, nor destroying the tag, returned an error code, and if so would throw an exception. I'm not sure if there are any valid reasons for return an error code (other than bugs in the wrapper or core library), and I believe there is no recourse if either fail.

The new code fixes the bug that was there (found in #432) and also ignores any other errors that might occur.

Your thoughts?

@timyhac timyhac merged commit 0fdbc61 into main Jan 29, 2025
2 checks passed
@timyhac timyhac deleted the #432 branch January 29, 2025 07:40
@iancolledge
Copy link

I can't thank you enough for not throwing that exception in the destructor. It was the source of many issues in live running systems when thrown from the finalizer invocation by the garbage collector thread. As you probably know exceptions from the GC thread are awful and cannot be handled like normal exceptions.
We think the source of the bug is either in the locking in the c lib, or the hashtable implementation only scanning 10 location ahead to deal with collisions. Either way it's gone now hopefully, but we're a bit worried if it's due the 10 iterations limit in the C lib there may be tags hanging around that never get cleaned up. We did some memory monitoring and it can't be that many. I raised a request for a method to query the hashtable size, but understand if you don't want to implement this. For now we will just restart our processes once a month.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants