Review API from Revamp PR #107

vuplea · 2024-10-23T13:04:44Z

No description provided.

vuplea · 2024-10-23T13:06:55Z

UiPath.Ipc.net6.0-windows.received.txt

+    public sealed class ClientConfig : UiPath.Ipc.EndpointConfig, System.IEquatable<UiPath.Ipc.ClientConfig>
+    {
+        public ClientConfig() { }
+        public string DebugName { get; set; }


not clear what DebugName does.

It's a CoreIpc-traditional tagging mechanism for reading logs and understanding who's who.
I didn't like how Name sounded (it seemed more functional than it actually was), so I prefixed it with Debug.

On support/v21.10 the public ones were ListenerSettings.Name and Connection.Name, both of which are public, and which spin off into internals (i.e. ServiceClient inherits the Name from it's associated Listener if it's of the callback sort) and all end up in logs, from hundreds of places, i.e: this

UiPath.Ipc.net6.0-windows.received.txt

vuplea · 2024-10-23T13:09:52Z

UiPath.Ipc.net6.0-windows.received.txt

+    {
+        protected ClientTransport() { }
+        public abstract UiPath.Ipc.IClientState CreateState();
+        public abstract void Validate();


maybe we should think if we really need the Validate feature?

UiPath.Ipc.net6.0-windows.received.txt

vuplea · 2024-10-23T13:12:33Z

UiPath.Ipc.net6.0-windows.received.txt

+        public System.TimeSpan RequestTimeout { get; init; }
+    }
+    [System.Serializable]
+    public sealed class EndpointNotFoundException : System.ArgumentOutOfRangeException


not an argument out of range problem

argument out of range is if we could know statically that the argument is outside of the input domain

vuplea · 2024-10-24T16:44:44Z

UiPath.Ipc.net6.0-windows.received.txt

+    public abstract class ListenerConfig : UiPath.Ipc.EndpointConfig, System.IEquatable<UiPath.Ipc.ListenerConfig>
+    {
+        protected ListenerConfig() { }
+        public System.Security.Cryptography.X509Certificates.X509Certificate? Certificate { get; init; }


was this in CoreIpc?
why do we expose this? is it for websocket only?

Yes it was, and it was agnostic than merely websockets;

I was wondering the same thing.

We use CoreIpc over Tcp/Ipc, but I can't find a precise usage for this certificate thing.
We have 0 websocket usage.

vuplea · 2024-10-24T16:51:59Z

UiPath.Ipc.net6.0-windows.received.txt

+        [return: System.Diagnostics.CodeAnalysis.NotNullIfNotNull("exception")]
+        public static UiPath.Ipc.Error? FromException(System.Exception? exception) { }
+    }
+    public interface IClient


IClientConnection name would be better:

we have the usecase in which we identify the same connection in hashset

the callback is only valid for that client connection, not if recreated

Sounds good, but we'll have to discuss whether what you said still holds.

vuplea · 2024-10-24T16:58:47Z

UiPath.Ipc.net6.0-windows.received.txt

+        public UiPath.Ipc.IClient Client { get; set; }
+        [Newtonsoft.Json.JsonIgnore]
+        public System.TimeSpan RequestTimeout { get; set; }
+        public TCallbackInterface GetCallback<TCallbackInterface>()


maybe it wouldn't be that bad to force the user to go through IClient(Connection) 😓 . it tells you more e.g. how long the callback is valid, but mostly for the reduced surface area.

vuplea · 2024-10-25T09:20:15Z

UiPath.Ipc.net6.0-windows.received.txt

+            "your compiler.", true)]
+        public IpcServer() { }
+        public UiPath.Ipc.EndpointCollection Endpoints { get; init; }
+        public System.Collections.Generic.IReadOnlyList<UiPath.Ipc.ListenerConfig> Listeners { get; init; }


I think we can improve the entity model to have better matching between client and server, and generally to simplify it.
The only real relationship differences I can see:

A client has a single connection while a listener has many. I don't see any problem with the API regarding this.

A server can have multiple transport configurations "listeners". Of note, we could drop this feature of multiple listeners. A user could simply create more servers calling the same configuration code.

I'd propose this model:
Endpoints = { Client, Server }
Endpoint = (IServiceProvider, Scheduler, RequestTimeout)
Client : Endpoint + (ClientTransport)
Server : Endpoint + (ServerTransport)

vuplea · 2024-10-25T09:28:41Z

UiPath.Ipc.net6.0-windows.received.txt

+}
+namespace UiPath.Ipc.Extensibility
+{
+    public interface IListenerConfig<TSelf, TListenerState, TConnectionState>


IListenerConfig vs ListenerConfig not obvious.
Why other namespace not obvious.
Maybe you'd want to move all transport stuff into UiPath.Ipc.Transport (client included), but I don't see the need.

Too many generics, one of the problems we wanted to avoid.

I agree, and we need to discuss it a bit, but this is exclusively for the select few who will do (maybe) a Blazor JSInterop transport.

vuplea · 2024-11-27T12:08:46Z

UiPath.Ipc.net6.0-windows.received.txt

+                "_accepter"})]
+        public void Start() { }
+        public System.Threading.Tasks.Task WaitForStart() { }
+        public System.Threading.Tasks.Task WaitForStop() { }


what;s the differentce between WaitForStop() and DisposeAsync()?
I'd like to avoid supporting the "restart" scenario.

vuplea · 2024-11-27T12:11:47Z

UiPath.Ipc.net6.0-windows.received.txt

+        public System.Threading.Tasks.TaskScheduler? Scheduler { get; set; }
+        public object? ServiceInstance { get; }
+        public System.IServiceProvider? ServiceProvider { get; }
+        public void Validate() { }


I see you've added ServiceProvider and scheduler to IpcBase - so we'd be good to remove them from here, right?

- other simplifications

- decommission WaitForStop

mciureanu · 2025-01-27T17:09:10Z

UiPath.Ipc.net6.0-windows.received.txt

+        public bool NewConnection { get; }
+    }
+    public abstract class ClientTransport : System.IEquatable<UiPath.Ipc.ClientTransport> { }
+    public class ContractCollection : System.Collections.Generic.IEnumerable<UiPath.Ipc.ContractSettings>, System.Collections.IEnumerable


I guess it's debatable, and based on preference, but if we would have builder methods to add "contracts", for example AddServiceType(Type...) and AddServiceInstance(Type, object), we wouldn't have to expose the ContractCollection class and its dependencies (ContractSettings). Also, the API using fluent builder methods would be easier to use for me, as a CoreIPC user. But again - it's a matter of opinion.

mciureanu · 2025-01-27T17:29:14Z

UiPath.Ipc.net6.0-windows.received.txt

+}
+namespace UiPath.Ipc.Transport.NamedPipe
+{
+    public sealed class NamedPipeClientTransport : UiPath.Ipc.ClientTransport, System.IEquatable<UiPath.Ipc.Transport.NamedPipe.NamedPipeClientTransport>


We could avoid exposing the whole Transport hierarchy (with abstract classes and all). Even if they look like only "configuration objects", in reality they are the runtime classes. It would be easier/cleaner to add fluent builder interfaces when building the IpcServer and IpcClient and hide the transport classes. Again - more of an opinion.

mciureanu · 2025-01-28T09:08:29Z

UiPath.Ipc.net6.0-windows.received.txt

+                "Transport",
+                "_accepter"})]
+        public void Start() { }
+        public System.Threading.Tasks.Task WaitForStart() { }


Do we need WaitForStart anywhere? If we do, what about Start being async?

As a CoreIpc server user, how can I know if a client connected or disconnected?
Or how can I simply send something to the client, without this operation being triggered by a request from the client?

It seems these are missing, no? Maybe not required by Robot, but seems pretty basic.

mciureanu · 2025-01-28T10:32:52Z

UiPath.Ipc.net6.0-windows.received.txt

+        protected IpcBase() { }
+        public System.TimeSpan RequestTimeout { get; set; }
+        public System.Threading.Tasks.TaskScheduler? Scheduler { get; set; }
+        public System.IServiceProvider? ServiceProvider { get; set; }


ServiceProvider is mandatory when we don't provide instances for Enpoints or Callbacks but types/interfaces? It's not obvious when building the IpcClient/IpcServer, and we will get a runtime error much later (not during building phase).

Personal opinion: We shouldn't bother with dependency injection in CoreIPC, it's not its responsibility. As a user of CoreIPC, even if I use dependency injection, I would probably want to use constructor injection myself for a service that creates the IpcServer, and give the instances to the IpcServer myself. It's also a matter of responsibilities. CoreIPC shouldn't have the responsibility of building service instances itself. The fact that objects are either given by me, or by the service provider seems also a little bit inconsistent.

But I guess the problem is deeper, because the IpcServer is just a declarative method, and we might want to create a new service instance for each client, but this is abstracted away from the user, so this change might require some rethinking, and a long discussion.

Example how I would do this:

var server = new Server (...);
server.OnClientConnected( (client) =>
{
client.RegisterService(calculatorService);
client.GetProxy() ... etc... }
}

then we can take the client and add it into a list, and so on...

Very broadly speaking, with this design, CoreIPC makes a step towards a high level library like SIgnalR, but it's a very small step, and it's not enough, and the downside is that it's hiding all the low level stuff.

Uploaded API from Revamp PR - commit 6b263e6

d632c88

vuplea commented Oct 25, 2024

View reviewed changes

taking shape

af72ff8

vuplea commented Nov 27, 2024

View reviewed changes

eduard-dumitru force-pushed the review_revamp_api branch from 1150dc4 to 9f3573b Compare November 27, 2024 22:13

rename Endpoint.. to Contract.. (..Collection, ..Settings)

0339141

- other simplifications

eduard-dumitru force-pushed the review_revamp_api branch from 9f3573b to 0339141 Compare November 27, 2024 22:21

simplifications after review

26eccce

- decommission WaitForStop

mciureanu reviewed Jan 27, 2025

View reviewed changes

mciureanu reviewed Jan 28, 2025

View reviewed changes

Review API from Revamp PR #107

Are you sure you want to change the base?

Review API from Revamp PR #107

Uh oh!

Conversation

vuplea commented Oct 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eduard-dumitru Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vuplea Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eduard-dumitru Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mciureanu Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mciureanu Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eduard-dumitru Nov 5, 2024 •

edited

Loading

vuplea Oct 23, 2024 •

edited

Loading

eduard-dumitru Nov 5, 2024 •

edited

Loading

mciureanu Jan 27, 2025 •

edited

Loading

mciureanu Jan 28, 2025 •

edited

Loading