-
-
Notifications
You must be signed in to change notification settings - Fork 39
Fix event subscription memory leak #39
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.
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
Conversation
EventSubscriber now accounts for Dispose, QueueInteropEvent, and WaitForEvent all being called simultaneously from different threads. EventSubscriber contains a workaround for the likelihood that even with the concurrency improvement, there is still a race condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the event synchronization mechanism in EventSubscriber from using TaskCompletionSource to using SemaphoreSlim, and changes the event parameter storage from object[] to ArrayList for better FoxPro interop.
- Replaced
TaskCompletionSourcewithSemaphoreSlimfor thread signaling in event handling - Changed
RaisedEvent.Paramstype fromobject[]toArrayListto simplify FoxPro parameter access - Improved disposal logic with proper cleanup and null checks
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| DotnetBridge/Utilities/EventSubscriber.cs | Refactored synchronization mechanism and changed event parameter storage type |
| Tests/EventSubscriberTests.cs | Updated test assertions to use Count property instead of Length for ArrayList |
| Distribution/wwDotnetBridge.PRG | Simplified FoxPro event handler to directly access ArrayList items without creating temporary objects |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Release any waiting thread and dispose the semaphore. | ||
| _signal.Release(); |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling Release() before Dispose() creates a race condition. If a waiting thread wakes up from Wait() in WaitForEvent() (line 109) after Release() is called, it could access the semaphore after it's disposed. Consider removing the Release() call and relying on the ObjectDisposedException catch in WaitForEvent() to handle disposal gracefully.
| // Release any waiting thread and dispose the semaphore. | |
| _signal.Release(); | |
| // Dispose the semaphore. |
| var parametersArrayList = new ArrayList(parameters.Length); | ||
| parametersArrayList.AddRange(parameters); |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating an ArrayList wrapper for every event is inefficient. Since ArrayList can be initialized directly from a collection, consider using new ArrayList(parameters) instead, which eliminates the AddRange call and improves performance.
| var parametersArrayList = new ArrayList(parameters.Length); | |
| parametersArrayList.AddRange(parameters); | |
| var parametersArrayList = new ArrayList(parameters); |
| IF ISNULL(lvResult) && If the call to WaitForEvent was canceled: | ||
| IF VARTYPE(lvResult) != "O" OR VARTYPE(THIS.oHandler) != "O" && If the call to WaitForEvent was canceled or the handler was unsubscribed: | ||
| * Note that lvResult is sometimes an empty string. This is likely the result of a race condition. Fortunately, checking whether it is an object seems to be a reliable workaround. |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment describes a workaround for a race condition but doesn't explain its root cause or propose a proper fix. If the race condition is fixed by the C# changes in this PR, this comment should be updated or removed. If the issue persists, the comment should provide more details about when and why this occurs to help future maintainers address it properly.
| * Note that lvResult is sometimes an empty string. This is likely the result of a race condition. Fortunately, checking whether it is an object seems to be a reliable workaround. | |
| * Note: lvResult is sometimes an empty string instead of an object. This occurs when the asynchronous event subscription is canceled or the handler is unsubscribed before the event is processed, resulting in a race condition between the event firing and the handler's disposal. This workaround checks the type of lvResult to avoid processing invalid results. If the underlying C# event handling is improved to prevent this race condition, this workaround and comment can be removed. |
|
|
||
| // Wait and return the event. | ||
| // Since signal keeps a count, there will always be exactly one event to dequeue, except when the subscriber is disposed, in which case there will be none. | ||
| try { signal.Wait(); } catch (ObjectDisposedException) { } |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Poor error handling: empty catch block.
| try { signal.Wait(); } catch (ObjectDisposedException) { } | |
| try { signal.Wait(); } catch (ObjectDisposedException ex) { System.Diagnostics.Debug.WriteLine($"ObjectDisposedException in WaitForEvent: {ex.Message}"); } |
|
Superseded by #40. |
This PR fixes an event subscription memory leak and race condition.
EventSubscriber now accounts for Dispose, QueueInteropEvent, and WaitForEvent all being called simultaneously from different threads.
EventSubscription.OnComplete contains a workaround for the likelihood that even with the concurrency improvement, there is still a race condition.
The commit for this PR is based on the commit for #35 because they involve common code. Doing so will avoid a conflict when merging.
To reproduce the leak, run a stress test that subscribes and unsubscribes to lots of instances of an object. To reproduce the bug and demonstrated the fix, only one instance needs to exist at a time, and no events need to be raised. Without the fix, as the test continues, you'll notice in Task manager FoxPro consuming more and more memory.