-
-
Notifications
You must be signed in to change notification settings - Fork 39
Add synchronization context #40
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
* Supports posting methods to the synchronization context. * Events by default are forwarded directly to the handler on the same thread. However, they can be configured to be posted (useful for events raised on a background thread or to avoid reentrant access). * When a task method completes, the status is posted on the main thread. * C# async/await uses the synchronization context by default, causing continuations to run on the main thread. .NET code that can run safely on a background thread can opt to do so with `ConfigureAwait(false)`. This commit eliminates background thread calls into FoxPro and C# continuations, avoiding race conditions and data tearing, and providing a deterministic runtime environment.
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 adds support for synchronizing .NET method calls and events with the FoxPro main thread using a custom synchronization context. This enables posting .NET method invocations to the FoxPro message queue to avoid reentrancy issues in event handlers and handle callbacks from background threads.
Key changes:
- Introduced
FoxProSynchronizationContextto marshal .NET callbacks to the FoxPro main thread via Windows messages - Added
PostMethodandPostStaticMethodfunctions to queue method invocations instead of executing them immediately - Refactored
EventSubscriberto support posting events to the synchronization context and simplified the event subscription model
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| DotnetBridge/wwDotNetBridge.csproj | Enabled latest C# language version to support modern syntax features |
| DotnetBridge/wwDotNetBridge.cs | Added synchronization context setup, posting support for method invocations, and reorganized using statements |
| DotnetBridge/Utilities/FoxProSynchronizationContext.cs | New class implementing SynchronizationContext for FoxPro thread marshaling |
| DotnetBridge/Utilities/EventSubscriber.cs | Refactored event subscription to support synchronization context posting and simplified event handling |
| Distribution/wwDotnetBridge.PRG | Added FoxPro-side synchronization context setup, new PostMethod/PostStaticMethod functions, and updated event subscription API |
Comments suppressed due to low confidence (4)
DotnetBridge/Utilities/FoxProSynchronizationContext.cs:39
- Poor error handling: empty catch block.
try { postedEvent.handler(postedEvent.state); } catch { }
DotnetBridge/wwDotNetBridge.cs:770
if (args[i] is ComValue)
DotnetBridge/wwDotNetBridge.cs:1038
if (args[i] is ComValue)
DotnetBridge/Utilities/FoxProSynchronizationContext.cs:39
- Generic catch clause.
try { postedEvent.handler(postedEvent.state); } catch { }
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| /// <summary> | ||
| /// Sets the current synchronization content to use the FoxPro main thread. |
Copilot
AI
Nov 2, 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.
Corrected spelling of 'content' to 'context' in the documentation comment.
| /// Sets the current synchronization content to use the FoxPro main thread. | |
| /// Sets the current synchronization context to use the FoxPro main thread. |
| return _synchronizationContext.PostMessageId; | ||
| } | ||
|
|
||
| public void Dispatch() => _synchronizationContext.Dispatch(); |
Copilot
AI
Nov 2, 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.
Potential null reference exception. The _synchronizationContext field can be null if SetSynchronizationContext is never called, but Dispatch() dereferences it unconditionally. Add a null check or document that SetSynchronizationContext must be called first.
| public void Dispatch() => _synchronizationContext.Dispatch(); | |
| public void Dispatch() | |
| { | |
| if (_synchronizationContext == null) | |
| throw new InvalidOperationException("SynchronizationContext has not been set. Call SetSynchronizationContext() before Dispatch()."); | |
| _synchronizationContext.Dispatch(); | |
| } |
| internal object InvokeStaticMethod_Internal(string TypeName, string method, params object[] args) | ||
| { | ||
| if (PostInvokedMethods) | ||
| { |
Copilot
AI
Nov 2, 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.
Potential null reference exception. When PostInvokedMethods is true, _synchronizationContext is accessed but may be null if SetSynchronizationContext was not called. Add a null check or validation.
| { | |
| { | |
| if (_synchronizationContext == null) | |
| throw new InvalidOperationException("SynchronizationContext is not set. Call SetSynchronizationContext before using PostInvokedMethods."); |
| if (PostInvokedMethods) | ||
| { | ||
| PostMethod_Internal(instance, method, args); | ||
| return null; |
Copilot
AI
Nov 2, 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.
Potential null reference exception in PostMethod_Internal. When PostInvokedMethods is true, the method calls _synchronizationContext.Post which will fail with a null reference if SetSynchronizationContext was not called.
| sealed class FoxProSynchronizationContext(int hwnd, object taskWaiter) : SynchronizationContext | ||
| { | ||
| private readonly IntPtr _hwnd = (IntPtr)hwnd; | ||
| private readonly ConcurrentQueue<(SendOrPostCallback handler, object? state)> _postQueue = []; | ||
| private readonly dynamic _taskWaiter = taskWaiter; |
Copilot
AI
Nov 2, 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.
The _taskWaiter field is declared but never used in the class. Consider removing it or documenting its intended future use.
| sealed class FoxProSynchronizationContext(int hwnd, object taskWaiter) : SynchronizationContext | |
| { | |
| private readonly IntPtr _hwnd = (IntPtr)hwnd; | |
| private readonly ConcurrentQueue<(SendOrPostCallback handler, object? state)> _postQueue = []; | |
| private readonly dynamic _taskWaiter = taskWaiter; | |
| sealed class FoxProSynchronizationContext(int hwnd) : SynchronizationContext | |
| { | |
| private readonly IntPtr _hwnd = (IntPtr)hwnd; | |
| private readonly ConcurrentQueue<(SendOrPostCallback handler, object? state)> _postQueue = []; |
| *** loHandler: An object with a method OnEvent(loEventName, loParams). | ||
| *** loHandler: An object with handler methods corresponding to each event to subscribe to. | ||
| *** lcPrefix: The initial part of the event handler function for each event. Defaults to "On". | ||
| *** llPost: When true, events are posted to the synchronization context. Useful for events raised on a background thread or to avoid reentrency. |
Copilot
AI
Nov 2, 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.
Corrected spelling of 'reentrency' to 'reentrancy'.
| *** llPost: When true, events are posted to the synchronization context. Useful for events raised on a background thread or to avoid reentrency. | |
| *** llPost: When true, events are posted to the synchronization context. Useful for events raised on a background thread or to avoid reentrancy. |
| /// <summary> | ||
| /// Gets the ID of the Windows message posted when a task is ready to run. | ||
| /// </summary> | ||
| public int PostMessageId { get; private set; } = RegisterWindowMessage("FoxProSynchronizationContextDispatch"); |
Copilot
AI
Nov 2, 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.
Replace this call with a call to managed code if possible.
| public override void Post(SendOrPostCallback d, object? state) | ||
| { | ||
| _postQueue.Enqueue((d, state)); | ||
| PostMessage(_hwnd, PostMessageId, IntPtr.Zero, IntPtr.Zero); |
Copilot
AI
Nov 2, 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.
Replace this call with a call to managed code if possible.
|
|
||
| [DefaultDllImportSearchPaths(DllImportSearchPath.System32)] | ||
| [DllImport("user32.dll", CharSet = CharSet.Unicode, BestFitMapping = false, ThrowOnUnmappableChar = true)] | ||
| private static extern int RegisterWindowMessage(string lpString); |
Copilot
AI
Nov 2, 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.
Minimise the use of unmanaged code.
|
|
||
| [DefaultDllImportSearchPaths(DllImportSearchPath.System32)] | ||
| [DllImport("user32.dll", SetLastError = true)] | ||
| private static extern bool PostMessage(IntPtr hWnd, int msg, IntPtr wParam, IntPtr lParam); |
Copilot
AI
Nov 2, 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.
Minimise the use of unmanaged code.
|
Superseded by #42. (The branch name was spelled wrong, and Github closed the PR when it got renamed.} |
ConfigureAwait(false).This PR eliminates background thread calls into FoxPro and C# continuations, avoiding race conditions and data tearing, and providing a deterministic runtime environment.