feat: Add "Once" socket listener registration for one-time handlers

[Denny09310]

This PR updates the socket listener handling to use the already existing Once method rather than manually adding and removing event handlers. This simplifies the code and reduces unnecessary listener cleanup logic while keeping the behavior the same.

[Denny09310]

The commit d8062aa was made because the test "GetAllDisplays_returns_at_least_one" was failing due to a Json deserialization mismatch

[FlorianRappl]

Great! LGTM!

[FlorianRappl]

Please have a look at this one @agracio.

[agracio]

I can see that it is another major source of conflicts for my change but unable to comment on viability of Once vs On->Off, this is something for @softworkz to look at.

I am assuming that there was some reason behind using On->Off but not sure who is able to shed some light on it.
Is Once a relatively new addition to SocketIO .NET implementation?

[FlorianRappl]

I am assuming that there was some reason behind using On->Off but not sure who is able to shed some light on it.
Is Once a relatively new addition to SocketIO .NET implementation?

That's my guess, too - but I must be blind to see that in the SocketIO GitHub source code ... For the OP question / discussion see #919

[FlorianRappl]

Source for the Once is the facade: https://github.com/ElectronNET/Electron.NET/blob/develop/src/ElectronNET.API/Bridge/SocketIOFacade.cs#L85

But honestly, it would change semantics. First, the lock in there is inconsistent (it locks when adding the event handler, but it does not lock when removing the handler). Second, the value is always set via a Task.Run.

Long story short I am not sure if we should accept this. I think it's much better to do #918 and do the API right beforehand.

[agracio]

There is nothing in changelog to indicate that Once was not a member of SocketIO from the very beginning.

[FlorianRappl]

There is nothing in changelog to indicate that Once was not a member of SocketIO from the very beginning.

Yeah it entered the code with the facade, i.e., with the recent rework / ElectronNET.Core.

[agracio]

From logs I can see that it was moved during rework and previously was part of API folder and always included Once?

[agracio]

SocketIOFacade.cs created on 23rd of March 2023 and added Once<T> method.
Before that change BridgeConnector was using Socket directly and presumably it does not have Once since SocketIOFacade explicitly created one.

[agracio]

Second, the value is always set via a Task.Run.

To be fair this is also true for On<T> - but there there is a subtle difference in locks between Once<T> and PropertyGetter implementation in ApiBase (from line 156). Lock is introduced after Off event.

Definitely struggling to judge what real life implications could be.

Long story short I am not sure if we should accept this. I think it's much better to do #918 and do the API right beforehand.

#918 focuses on removing ApiEventManager as well as moving some more of the tasks to use GetPropertyAsync<T> from ApiBase.

Major source of conflicts is complete removal of ApiEventManager as well as multiple changes to tasks that switched to using GetPropertyAsync<T>.

[agracio]

@FlorianRappl off topic for this PR but you mentioned messaging apps before, I am not on Gitter but available on Slack and Discord.
Easy to find on Slack (wink wink) but not on Discord.

[softworkz]

Looking all good to me!

[softworkz]

There is nothing in changelog to indicate that Once was not a member of SocketIO from the very beginning.

Yeah it entered the code with the facade, i.e., with the recent rework / ElectronNET.Core.

It looks like that, because my moving of files unfortunately broke the Git history of SocketIOFacade. @agracio is right - SocketIOFacade has been introduced in 2023 by @GregorBiswanger. What I can see is that he did a migration from Quobject.SocketIoClientDotNet to SocketIO. To avoid refactoring existing code, he introduced SocketIOFacade as an adaption layer due to the different API surface of SocketIO. Quobject.SocketIoClientDotNet did have a Once() method while SocketIO does not, so had added a custom implementation of it.

Task.Run() in Event Handlers

A very common issue with events (not even limited to .net) is that often neither the library developer (who fires it) nor the developer of the consuming code (who's handing it) are thinking about what kind of thread the event invocation is running on and about the consequences for either side: the handling-side developer may do tons of things and long-running operation without caring about what it might mean for the source-side of the even when the invocation doesn't return quickly and the lib side doesn't consider that the event trigger might not return immediately.

In this regard, @GregorBiswanger has done the right thing: using Task.Run() to decouble the handling code and return from the actual handler quickly.
It is no longer needed today, because SocketIO is doing the same:

doghappy/socket.io-client-csharp@68abd98#diff-06140f86f2dc1f9b6094785b252cce139ca8c239533b4002199c0d1c49dcde52L20-L30

But Gregor's commit was in Feb 2023 and at that time, SocketIO were still firing on their own thread. The referenced change was made 6 months after Gregor's commit.