ApiEventManager Rework

[softworkz]

Figured out the problem

internal static void AddEvent(string eventName, object id, Action callback, Action value)
{
    var call = (Action)events.GetOrAdd(eventName, callback);
    if (call == null)
    {
        BridgeConnector.Socket.On(eventName + id, () =>
        {
            ((Action)events[eventName])();
        });
        BridgeConnector.Socket.Emit($"register-{eventName}", id);
    }
    
    call += value;
    events[eventName] = call;
}

@softworkz could you advise whether it requires any locks and if so where.

Originally posted by @agracio in #913 (comment)

[softworkz]

@agracio - This was much harder than I had thought and I had to try it out myself to be sure.

Below is a new version of the ApiEventManager which should be solid, elegant, easy-to-understand and low-code.

Notes

• Revert the Screen (1) and Tray (3) events to the original implementation
  It's not worth to introduce common handling for those 4 cases
• What's next to do would be to
  • Remove the converter parameter, converters are no longer needed
  • Remove the callback parameter (not needed)
  • Remove all the private event variables that went into the callback parameter

using System;
using System.Collections.Concurrent;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using ElectronNET.API;

namespace ElectronNET.Common;

internal static class ApiEventManager
{
    private static readonly object LockObj = new object();
    private static readonly ConcurrentDictionary<string, EventContainer> EventContainers = new();

    internal static void AddEvent(string eventName, int? id, Action callback, Action value)
    {
        var eventKey = EventKey(eventName, id);

        lock (LockObj)
        {
            var container = EventContainers.GetOrAdd(eventKey, _ =>
            {
                var container = new EventContainer();
                BridgeConnector.Socket.On(eventKey, container.OnEventAction);
                BridgeConnector.Socket.Emit($"register-{eventName}", id);
                return container;
            });

            container.Register(value);
        }
    }

    internal static void RemoveEvent(string eventName, int? id, Action callback, Action value)
    {
        var eventKey = EventKey(eventName, id);

        lock (LockObj)
        {
            if (EventContainers.TryGetValue(eventKey, out var container) && !container.Unregister(value))
            {
                BridgeConnector.Socket.Off(eventKey);
                EventContainers.TryRemove(eventKey, out _);
            }
        }
    }


    internal static void AddEvent<T>(string eventName, int? id, Action<T> callback, Action<T> value, Func<object, T> converter = null)
    {
        var eventKey = EventKey(eventName, id);

        lock (LockObj)
        {
            var container = EventContainers.GetOrAdd(eventKey, _ =>
            {
                var container = new EventContainer();
                BridgeConnector.Socket.On<T>(eventKey, container.OnEventActionT);
                BridgeConnector.Socket.Emit($"register-{eventName}", id);
                return container;
            });

            container.Register(value);
        }
    }

    internal static void RemoveEvent<T>(string eventName, int? id, Action<T> callback, Action<T> value)
    {
        var eventKey = EventKey(eventName, id);

        lock (LockObj)
        {
            if (EventContainers.TryGetValue(eventKey, out var container) && !container.Unregister(value))
            {
                BridgeConnector.Socket.Off(eventKey);
                EventContainers.TryRemove(eventKey, out _);
            }
        }
    }

    private static string EventKey(string eventName, int? id)
    {
        return string.Format(CultureInfo.InvariantCulture, "{0}{1:D}", eventName, id);
    }

    [SuppressMessage("ReSharper", "InconsistentlySynchronizedField")]
    private class EventContainer
    {
        private Action eventAction;
        private Delegate eventActionT;

        private Action<T> GetEventActionT<T>()
        {
            return (Action<T>)this.eventActionT;
        }

        private void SetEventActionT<T>(Action<T> actionT)
        {
            this.eventActionT = actionT;
        }

        public void OnEventAction() => this.eventAction?.Invoke();

        public void OnEventActionT<T>(T p) => this.GetEventActionT<T>()?.Invoke(p);

        public void Register(Action receiver)
        {
            this.eventAction += receiver;
        }

        public void Register<T>(Action<T> receiver)
        {
            var actionT = this.GetEventActionT<T>();
            actionT += receiver;
            this.SetEventActionT(actionT);
        }

        public bool Unregister(Action receiver)
        {
            this.eventAction -= receiver;
            return this.eventAction != null;
        }

        public bool Unregister<T>(Action<T> receiver)
        {
            var actionT = this.GetEventActionT<T>();
            actionT -= receiver;
            this.SetEventActionT(actionT);

            return actionT != null;
        }
    }
}

[softworkz]

PS: I created this new issue because the PR isn't really a good place for discussing this. 😉

[agracio]

I have done the following: https://github.com/agracio/Electron.NET/blob/develop/src/ElectronNET.API/API/ApiBase.cs
Scroll to line 126.

The only thing that is missing is locks in my opinion.

Remove the converter parameter, converters are no longer needed

I will be happy to do that but are you sure Socket can handle correct serialization/deserialization? The only reason I have converters is because that is how it is used in original code.

Example

public event Action<UpdateInfo> OnUpdateAvailable
{
    add => ApiEventManager.AddEvent("autoUpdater-update-available", GetHashCode(), _updateAvailable, value, (args) => JObject.Parse(args.ToString()).ToObject<UpdateInfo>());
    remove => ApiEventManager.RemoveEvent("autoUpdater-update-available", GetHashCode(), _updateAvailable, value);
}

public event Action<OnDidFailLoadInfo> OnDidFailLoad
{
    add => ApiEventManager.AddEvent("webContents-didFailLoad", Id, _didFailLoad, value, (args) => ((JObject)args).ToObject<OnDidFailLoadInfo>());
    remove => ApiEventManager.RemoveEvent("webContents-didFailLoad", Id, _didFailLoad, value);
}

[softworkz]

I have done the following: agracio/Electron.NET@develop/src/ElectronNET.API/API/ApiBase.cs
Scroll to line 126.

This is great, I would have moved it there as well and using CallerMemberName is cool on top.

The only thing that is missing is locks in my opinion.

And that is the problem. I've been sitting on this for 2.5h - almost since you had asked about locks.
Since you have already gone in the same direction, you just need to copy the EventContainer class into ApiBase and the 4 method bodies for Add/RemoveEvent.

You can try to figure out why I'm saying it's better, or I can explain tomorrow (I need a break now...)

Remove the converter parameter, converters are no longer needed

I will be happy to do that but are you sure Socket can handle correct serialization/deserialization? The only reason I have converters is because that is how it is used in original code.

The original code used the old SocketIO which couldn't properly handle it. For ElectronNET.Core, I had updated all libraries (but not the code).
I had tried without converters and all tests have still succeeded. I mean, you can also start by just ignoring the converters and see how it goes on your side.

Do you have something like ReSharper? Not that you start doing those removals by hand... 😆 ...it's a matter of 5 seconds.

[softworkz]

I want to stress that I like the changes you made. If you hadn't made me think about locking, I would have said that it's very good..

[agracio]

Do you have something like ReSharper?

Of course 😄

I mean it is Jetbrains Rider it could not be any other way.

[FlorianRappl]

Honestly, I think I can solve that problem by shifting it.

Let me explain: The main problem with the .NET code is that multi threading has always been a bit of pain. That's why many modern languages try to circumvent the issue by essentially only allowing code / variables to be touched by the owning thread (fiber, coroutine, whatever you want to call it - for sake of argument lets treat them as equal here). JavaScript for instance is single-threaded, with Node.js using a Reactor pattern to let all native code flow into that single thread.

That all being written - what if we just let the IPC counter part handle all that? Right now what we do:

• Event is added:
  • In .NET: Let's see if the event has been added, if not, let's register it
  • Add the handler
  • In Node.js: Only add event command is received - then always reply back to that one source
• Event is removed:
  • In .NET: Let's see if the event has only handlers left; if not - let's unregister it
  • Remove the handler
  • In Node.js: Remove event command is received - removal of Electron event handler

What I propose is to leave that in Node.js:

• Event is added:
  • In .NET: Let's send the command to register an event with the ID of the handler
  • In Node.js: Have a map with event names to handler IDs - if the given event name is not present let's start listening. The listener opens the entry in the map and sends the args to all registered handlers.
• Event is removed:
  • In .NET: Let's send the command to unregister an event with the ID of the handler
  • In Node.js: Have a map with event names to handler IDs - if the given event name is present let's remove the handler ID from the entry. The listener (and the entry) is fully removed if no handler IDs remain.

Result? Node.js handles the concurrent part, which it will do very well. In .NET we only have a simple logic to reason with.

Any thoughts on that one @softworkz @agracio ?

Remark: Improving the main.js / Node.js process can also be done to avoid having Electron processes floating around. Once the parent process ID is removed / no longer active the process could be shut down. This could maybe be also achieved with a periodic keep alive from the WebSocket channel. If the keep alive is missing, we'll close the process.

[agracio]

Remark: Improving the main.js / Node.js process can also be done to avoid having Electron processes floating around. Once the parent process ID is removed / no longer active the process could be shut down. This could maybe be also achieved with a periodic keep alive from the WebSocket channel. If the keep alive is missing, we'll close the process.

I was thinking that it can be done the same way but keep wondering if there is a more elegant solution. To be honest probably not.
In a few hours of writing code and running tests had about 15 hanging electron.exe processes.

As for improving IPC - no thoughts about it yet. Refactoring code tends to create a very narrow field of view for me until it is finished. In other words right now I don't see the forest for the trees.

Will try to get a better overall view some time later.

[softworkz]

You probably both missed my post #908 (comment) - but admittedly, it's not a good idea to spin long discussions in PRs. So here's sure better. Repost:

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.

[agracio]

The original code used the old SocketIO which couldn't properly handle it. For ElectronNET.Core, I had updated all libraries (but not the code).
I had tried without converters and all tests have still succeeded. I mean, you can also start by just ignoring the converters and see how it goes on your side.

Would it be able to handle enums?

var themeSourceValue = (ThemeSourceMode)Enum.Parse(typeof(ThemeSourceMode), (string)themeSource, true);

taskCompletionSource.SetResult(themeSourceValue);

Asking to make sure I can make changes.

EDIT: I will be adding tests alongside the changes so should be able to check soon.

[softworkz]

Would it be able to handle enums?
var themeSourceValue = (ThemeSourceMode)Enum.Parse(typeof(ThemeSourceMode), (string)themeSource, true);
taskCompletionSource.SetResult(themeSourceValue);

Like this? No (we have differences in casing and sometimes even more different names where the mapping is done via certain attributes on enum members. Enum.Parse() (better use the generic version) can do case-insensitive comparisons, but that's it then. Only a properly configured json serializer can do this.

But this should work:

Socket.On(....

[agracio]

public Task<ThemeSourceMode> GetThemeSourceAsync() => GetPropertyAsync<ThemeSourceMode>();

works perfectly and is included in test 😉