Mitigate race conditions and simplify API invocations

[softworkz]

There's a major design flaw in the API invocations.

The problem is that requests via socket IO are not numbered (like wiht a serial nr) and there can be only a single listener for a certain event name.
This means that when there's an invocation that is waiting for a return and a second invocation is made, the listener for the previous invocation will never be notified.
In combination with the use of TaskCompletionSource, the resulting behavior is that the tasks of such calls were never transitioned to completion (nor errored, nor canceled).

The result: code that was awaiting such tasks remained hanging forever - possibly even preventing the whole application from finishing gracefully (without needing to be forced to).

This PR simplifies those property invocations and it also mitigates those race conditions in a way that only a single invocation (= single TaskCompletionSource) exists for each method at a time. Subsequent calls are just being returned the same task from the TCS.
There can still be situations where there's no return, that's why the PR also introduces a timeout after which the tasks will be canceled. The duration might be debatable or can be made configurable. It's set to 1s at the moment.

Second benefit is the elimination of load of repetitive code. It reduces BrowserWindow from 1.7k to 1.2k lines.

[softworkz]

@agracio - what's funny is that I have done nothing about events, so it fits nicely together with your changes without any conflicts.

[Copilot]

Pull Request Overview

This pull request introduces a significant refactoring to reduce code duplication by implementing a base class pattern (ApiBase) for Electron.NET API classes. The PR consolidates repetitive socket communication and property getter logic into reusable helper methods.

• Introduces new ApiBase abstract class with helper methods for calling socket methods and retrieving properties
• Refactors BrowserWindow and App classes to inherit from ApiBase and use the new helper methods
• Adds utility extension methods LowerFirst and StripAsync to support the base class functionality

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 13 comments.

File	Description
src/ElectronNET.API/API/ApiBase.cs	New base class providing common socket communication patterns with timeout handling and method/property call abstractions
src/ElectronNET.API/API/BrowserWindow.cs	Refactored to extend ApiBase and replace verbose socket calls with concise helper method invocations
src/ElectronNET.API/API/App.cs	Refactored to extend ApiBase and replace verbose socket calls with concise helper method invocations
src/ElectronNET.API/Common/Extensions.cs	Added LowerFirst and StripAsync string extension methods; changed class visibility to internal; removed unused import

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[agracio]

I can see that you refactored a lot of tasks which is what I am doing now although I am touching considerably more files.
However I am not sure if it is correct, will go over the changes now.

In any case it appears I will have to roll back more than a day's worth of work.

[softworkz]

In any case it appears I will have to roll back more than a day's worth of work.

Oh, I'm sorry about that. But I suppose you didn't address the race conditions - which is crucial.

[agracio]

I am struggling to understand how tasks work now.

Example from BrowserWindow.cs

before

    public Task<string> GetTitleAsync()
    {
        var taskCompletionSource = new TaskCompletionSource<string>();

        BridgeConnector.Socket.On("browserWindow-getTitle-completed", (title) => {
            BridgeConnector.Socket.Off("browserWindow-getTitle-completed");

            taskCompletionSource.SetResult(title.ToString());
        });

        BridgeConnector.Socket.Emit("browserWindowGetTitle", Id);

        return taskCompletionSource.Task;
    }

after

public Task<string> GetTitleAsync() => this.GetPropertyAsync<string>();

Emit() signature "browserWindowGetTitle", On() signature "browserWindow-getTitle-completed".

Am I to dense to understand how it will be able to connect to JS backend.

socket.on('browserWindowGetTitle', (id) => {
    const title = getWindowById(id).getTitle();

    electronSocket.emit('browserWindow-getTitle-completed', title);
});

Same goes for the rest of changes.

[softworkz]

Am I to dense to understand how it will be able to connect to JS backend.

No, you probably just don't known about the CallerMemberName attribute.

Look at the GetPropertyAsync implementation:

        protected Task<T> GetPropertyAsync<T>([CallerMemberName] string callerName = null)
        {
            Debug.Assert(callerName != null, nameof(callerName) + " != null");

            return this.propertyGetters.GetOrAdd(callerName, _ =>
            {
                var getter = new PropertyGetter<T>(this, callerName, PropertyTimeout);

                getter.Task<T>().ContinueWith(_ => this.propertyGetters.TryRemove(callerName, out var _));

                return getter;
            }).Task<T>();
        }

The CallerMemberName attribute will fill the name of the calling method. That's the trick...

[agracio]

My understanding is that it will use the name of the calling method, isn't that correct.
But how does it translate to "browserWindowGetTitle" and "browserWindow-getTitle-completed", that is the part I am missing.

[softworkz]

That's in the PropertyGetter<T> class. See those lines:

var eventName = apiBase.propertyEventNames.GetOrAdd(callerName, s => $"{apiBase.objectName}-{s.StripAsync().LowerFirst()}{apiBase.SocketEventCompleteSuffix}");
var messageName = apiBase.propertyMessageNames.GetOrAdd(callerName, s => apiBase.objectName + s.StripAsync());

[softworkz]

The dictionaries there are just a means of optimization, so that the strings do not need to be recreated each time (imagine moveWindow or resizeWindow events where you might query the values thousands of times).

[agracio]

This is a very clever piece of code. However it does rely on the fact that all naming conventions are somewhat similar.

var eventName = apiBase.propertyEventNames.GetOrAdd(callerName, s => $"{apiBase.objectName}-{s.StripAsync().LowerFirst()}{apiBase.SocketEventCompleteSuffix}");

This code does account for the fact that there are multiple completion namings, so far I have seen 3 - -completed, -Completed and Completed.

You have it set incorrectly for BrowserWindow it should be -completed.

However this part of the code assumes that messageName is always constructed the same way.

var messageName = apiBase.propertyMessageNames.GetOrAdd(callerName, s => apiBase.objectName + s.StripAsync());

Unfortunately it can also be $"{apiBase.objectName}-{s.StripAsync().LowerFirst()}"

Screen.cs

        public Task<int> GetMenuBarHeightAsync()
        {
            var taskCompletionSource = new TaskCompletionSource<int>();

            BridgeConnector.Socket.On("screen-getMenuBarHeightCompleted", (height) =>
            {
                BridgeConnector.Socket.Off("screen-getMenuBarHeightCompleted");

                taskCompletionSource.SetResult(int.Parse(height.ToString()));
            });

            BridgeConnector.Socket.Emit("screen-getMenuBarHeight");

            return taskCompletionSource.Task;
        }

Not sure if that covers all of them, there could be more variations.

[softworkz]

You have it set incorrectly for BrowserWindow it should be -completed.

Ooops, you are right. In my working repo I have it correctly:

.

The differences between classes were the reason why I even added the SocketEventCompleteSuffix property.
When rebasing my code onto your, I had left out that property and later added the wrong one.

Thanks, good find! PR updated!

[softworkz]

Unfortunately it can also be $"{apiBase.objectName}-{s.StripAsync().LowerFirst()}"

True, that's why I didn't do the other classes..

[agracio]

True, that's why I didn't do the other classes..

One way to deal with it is to actually rename them. I have done it as part of events refactoring.

[softworkz]

True, that's why I didn't do the other classes..

One way to deal with it is to actually rename them. I have done it as part of events refactoring.

You mean on the TypeScript side? That's the best way of course.

If you want to continue this, it would be great, I have no plans to further work on this at the moment.

A less nice but pragmatic way, when there's another widespread pattern would be a second function like GetPropertyAsyncL1().. or similar (for lower first :wink)

[agracio]

Currently demo app is failing, easy to replicate by going to "Manage window state" clicking "View Demo" and trying to move new window. It is getting a bit late to investigate.

And after failure it does leave 3 orphaned electron.exe instances.

[softworkz]

Holy cow - the GetOrAdd() of ConcurrentDictionary is not synchronized (as I had thought). It's fixed now.

What this surfaced is also a problem with SocketIO client. Its methods are not thread-safe:

.

I've seen this before a number of times, but forgot to address it. I'Ve added a new commit to synchronize this in SocketIOFacade.

Thanks

[FlorianRappl]

LGTM - definitely a welcome change and some significant improvements here. Many thanks @softworkz !

[agracio]

There is a fork of this project that has significant changes to BridgeConnector, it appears to involve similar approach with dictionaries but moves it to a higher level meaning that both events and tasks and anything else that calls BridgeConnector is thread safe.
At least thats what it looks at first glance, have not gone through the entire code.

Take a look here https://github.com/theolivenbaum/electron-sharp/blob/main/ElectronSharp.API/BridgeConnector.cs.

The project also has very significant changes to SocketIO and serialization including some changes to SocketIo in main.js.

https://github.com/theolivenbaum/electron-sharp/tree/main/ElectronSharp.API/SocketIO

[agracio]

I will hold of on any changes while @softworkz is doing refactoring.
Once this is done I can migrate more tasks to new code by changing their signatures.

[agracio]

If you want to continue this, it would be great, I have no plans to further work on this at the moment.

@softworkz could you clarify what you mean by that, no more changes expected to task and Sockets code?

Could you take a look at BridgeConnector implementation in this repo https://github.com/theolivenbaum/electron-sharp/blob/main/ElectronSharp.API/BridgeConnector.cs and let me know if you think it is a good idea to implement it here.

There are very clear advantages of this code as it moves thread safety one level up encapsulating any calls to SocketIO regardless if it is made by events, tasks or anything else.

But if I was to look at implementing it in Electron.NET code I believe some it would mean removal of dictionaries and all the other thread safety features that you implemented in ApiBase.

I am more than happy to look into it but would like your opinion first.

[softworkz]

If you want to continue this, it would be great, I have no plans to further work on this at the moment.

@softworkz could you clarify what you mean by that, no more changes expected to task and Sockets code?

I made the changes in this PR three weeks ago already because I needed them (the PropertyGet part) for my actual project.
It's been just a quick change. I just did the base implementation and an example for each case, then had an AI do the rewriting of all the calls. It wasn't a big effort.
Generally, I might be not a good advisor at the moment, because my project is in private beta right now and I cannot afford making any changes that might have unexpected implications, that's why I didn't go for a more fundamental rework like it was done in the fork you referenced.
From a quick look, it appears that they are catching a wider range of races indeed. Probably still not all cases.
The extent of required trade-in for this approach is something that I cannot assess without taking a deeper look.

But I don't think that this matters a lot. When you look at my PropertyGet code and even more their SocketBridge modification and you realize how far all this needs to go in trying to bend things into a sane shape, what does it tell you?

If your conclusion is that something is very wrong - then that's spot on.

Do you know what it is?

[agracio]

I can only guess that thread syntonisation and SocketIO stability are the main issues.

Additionally none of the the changes made in this repo and changes in electron-sharp resolve an issue of orphan processes in case of dotnet exception. The only ideas I have regarding it are not the best.

Looking at electron-sharp code it appears to have a very comprehensive thread safety mechanics that include locks, semaphores and dictionaries to make sure events are are always synced and only one instance is used.

According to readme their solution was created for use in their product making it a very similar case to yours since you are making changes to create a working product.

The question is whether we want to pull their changes into Electron.NET repo - this would be a very big code change.

I could take a look at it but that would be a major change and since there are dozens of additional files that are included in electron-sharp SocketIO implementation validating every one of them would very hard so it would be as-is addition.

Please share your opinions on this.

[softworkz]

I can only guess that thread syntonisation and SocketIO stability are the main issues.

Well, those are the collaterals, but the source of all evil is something quite different:

It's the use of SocketIO named events, and trying to squeeze the behavior that we would actually need into the limitations and constraints of SocketIO events.
That is the major design - let's say 'flaw' - that is leading to all the problems that we (and the fork) have to fight against.

The current implementation tries to establish short-lived SocketIO event channels "per invocation":

• This (unnecessarily) introduces the limitation that a remote invocation under a given (Socket-)event name needs to be serialized (which is what my PropertyGet and the fork code is attempting to do):
  • There can be only a single ongoing invocation at a time, for example, when the client code wants to retrieve the size of multiple windows at the same time, the invocation for the 2nd window needs to wait until the invocation for the 1st window has returned and the invocation for the 3rd window needs to wait for the 1st and 2nd invocation to be completed, etc.
  • When one of the invocations doesn't return (for some reason), the others would be blocked forever (unless there's a timeout, but then the other invocations would still be blocked until the timeout is reached)
  • When there are many invocations for the same property on the same window, both approaches (fork and mine) are returning the task for the current outstanding invocation - so they'll get the same result as all previous pending implementations.
    That's only a band-aid, but not really a good solution, because when the subsequent invocations could be made in parallel, they might not receive the same return value than you get from the still pending invocation. This can be important when you need to synchronize window sizes and/or positions.
    In my case for example, I had to resort to using the native X-Window (x11Lib) API via P/Invoke in order to achieve a fast and reliable synchroniztation of windows on Linux (the were other reasons for this as well, though)
• The fact that On() and Off() are not even synchronized at the side of SocketIO shows that SocketIO was never even meant to be used that way

The Ideal Way

I have a bit of a problem talking about this, because I don't want to make any promises that I won't be able to fulfill, but as a matter of fact, I have done an implementation for bidirectional JS(browser) <=> CSharp projection which brings JS objects to C# and C# objects to JS. It supports method/function calling (async + sync from JS => C#), property retrieval and event registration (in both directions). It works cross-browser and "cross-ui-framework" (UWP/Xbox, WinUI w. WebView2, Electron, Android, iOS, Tizen/untested), battle-tested (approaching 1M installations) and high-performant (it can sync the movement of a CSS transition to a hWND native window on Windows).

Communication is message-based and all goes through a single channel. That would mean - in regards to SocketIO - there would be a single On('message') at app start and a single Off('message') on shutdown. Messages have a serial number, which means that - no matter how many invocations are active in parallel - each invocation will always get its own response, without any collisions or race conditions.

On the JS side, all invocations are be made directly using the JS function prototypes,, there's no need to create any proxy classes like al the TS/JS code under ElectronNET.Host/api - only some edge cases would need special/explicit handling.
At the C# side, the JS objects are represented by interfaces (which are autogenerated). At runtime, objects are materialized at runtime via System.Reflection.DispatchProxy.

So - that's a recipe that works and that I would use when building ElectronNET from scratch.
I don't think that I'll have time to adapt this for ElectronNET - but the messaging part alone could be implemented with a lot less effort. The TypeScript code rewriting could probably be tasked to an AI agent once the required architecture change is implemented.