Review API from Revamp PR #107
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,204 @@ | ||
| [assembly: System.Reflection.AssemblyMetadata("RepositoryUrl", "https://github.com/UiPath/coreipc.git")] | ||
| [assembly: System.Runtime.CompilerServices.InternalsVisibleTo("Playground")] | ||
| [assembly: System.Runtime.CompilerServices.InternalsVisibleTo("UiPath.CoreIpc.BackCompat")] | ||
| [assembly: System.Runtime.CompilerServices.InternalsVisibleTo("UiPath.CoreIpc.Extensions.Abstractions")] | ||
| [assembly: System.Runtime.CompilerServices.InternalsVisibleTo("UiPath.CoreIpc.Tests")] | ||
| [assembly: System.Runtime.CompilerServices.InternalsVisibleTo("UiPath.Ipc.Tests")] | ||
| [assembly: System.Runtime.Versioning.SupportedOSPlatform("Windows7.0")] | ||
| [assembly: System.Runtime.Versioning.TargetFramework(".NETCoreApp,Version=v6.0", FrameworkDisplayName=".NET 6.0")] | ||
| [assembly: System.Runtime.Versioning.TargetPlatform("Windows7.0")] | ||
| namespace UiPath.Ipc | ||
| { | ||
| public readonly struct CallInfo | ||
| { | ||
| public CallInfo(bool newConnection, System.Reflection.MethodInfo method, object?[] arguments) { } | ||
| public object?[] Arguments { get; } | ||
| public System.Reflection.MethodInfo Method { get; } | ||
| 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 | ||
| { | ||
| public ContractCollection() { } | ||
| public void Add(System.Type contractType) { } | ||
| public void Add(UiPath.Ipc.ContractSettings endpointSettings) { } | ||
| public void Add(System.Type contractType, object? instance) { } | ||
| public System.Collections.Generic.IEnumerator<UiPath.Ipc.ContractSettings> GetEnumerator() { } | ||
| } | ||
| public sealed class ContractSettings | ||
| { | ||
| public ContractSettings(System.Type contractType, System.IServiceProvider serviceProvider) { } | ||
| public ContractSettings(System.Type contractType, object? serviceInstance = null) { } | ||
| public System.Func<UiPath.Ipc.CallInfo, System.Threading.CancellationToken, System.Threading.Tasks.Task>? BeforeIncomingCall { get; set; } | ||
| public System.Threading.Tasks.TaskScheduler? Scheduler { get; set; } | ||
| } | ||
| public sealed class EndpointNotFoundException : System.ArgumentException | ||
| { | ||
| public string EndpointName { get; } | ||
| public string ServerDebugName { get; } | ||
|
There was a problem hiding this comment. Is this more like a "display name"? There was a problem hiding this comment. You're right; it's passed from the The Exception.Message becomes: internal static string FormatMessage(string serverDebugName, string endpointName)
=> $"Endpoint not found. Server was \"{serverDebugName}\". Endpoint was \"{endpointName}\".";There was a problem hiding this comment. but how does the value of "ServerDebugName" tie to what the user has configured? |
||
| } | ||
| public class Error : System.IEquatable<UiPath.Ipc.Error> | ||
| { | ||
| public Error(string Message, string StackTrace, string Type, UiPath.Ipc.Error? InnerError) { } | ||
| public UiPath.Ipc.Error? InnerError { get; init; } | ||
| public string Message { get; init; } | ||
| public string StackTrace { get; init; } | ||
| public string Type { get; init; } | ||
| public override string ToString() { } | ||
| [return: System.Diagnostics.CodeAnalysis.NotNullIfNotNull("exception")] | ||
| public static UiPath.Ipc.Error? FromException(System.Exception? exception) { } | ||
| } | ||
| public interface IClient | ||
|
There was a problem hiding this comment. IClientConnection name would be better:
There was a problem hiding this comment. Sounds good, but we'll have to discuss whether what you said still holds. |
||
| { | ||
| TCallbackInterface GetCallback<TCallbackInterface>() | ||
| where TCallbackInterface : class; | ||
| void Impersonate(System.Action action); | ||
| } | ||
| public static class IOHelpers | ||
| { | ||
| public static System.IO.Pipes.PipeSecurity Allow(this System.IO.Pipes.PipeSecurity pipeSecurity, System.Security.Principal.IdentityReference sid, System.IO.Pipes.PipeAccessRights pipeAccessRights) { } | ||
| public static System.IO.Pipes.PipeSecurity Allow(this System.IO.Pipes.PipeSecurity pipeSecurity, System.Security.Principal.WellKnownSidType sid, System.IO.Pipes.PipeAccessRights pipeAccessRights) { } | ||
| public static System.IO.Pipes.PipeSecurity AllowCurrentUser(this System.IO.Pipes.PipeSecurity pipeSecurity, bool onlyNonAdmin = false) { } | ||
| public static System.IO.Pipes.PipeSecurity Deny(this System.IO.Pipes.PipeSecurity pipeSecurity, System.Security.Principal.IdentityReference sid, System.IO.Pipes.PipeAccessRights pipeAccessRights) { } | ||
| public static System.IO.Pipes.PipeSecurity Deny(this System.IO.Pipes.PipeSecurity pipeSecurity, System.Security.Principal.WellKnownSidType sid, System.IO.Pipes.PipeAccessRights pipeAccessRights) { } | ||
| public static System.IO.Pipes.PipeSecurity LocalOnly(this System.IO.Pipes.PipeSecurity pipeSecurity) { } | ||
| [System.ComponentModel.Browsable(false)] | ||
| public static bool PipeExists(string pipeName, int timeout = 1) { } | ||
|
There was a problem hiding this comment. afaik we don't use PipeExists anymore, it came with a lot of problems - it showed false negatives. suggest removing it. |
||
| } | ||
| public abstract class IpcBase | ||
| { | ||
| protected IpcBase() { } | ||
| public System.TimeSpan RequestTimeout { get; set; } | ||
| public System.Threading.Tasks.TaskScheduler? Scheduler { get; set; } | ||
| public System.IServiceProvider? ServiceProvider { get; set; } | ||
|
There was a problem hiding this comment. 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 (...); 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. |
||
| } | ||
| public sealed class IpcClient : UiPath.Ipc.IpcBase | ||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public IpcClient() { } | ||
| public System.Func<System.Threading.CancellationToken, System.Threading.Tasks.Task>? BeforeConnect { get; set; } | ||
| public System.Func<UiPath.Ipc.CallInfo, System.Threading.CancellationToken, System.Threading.Tasks.Task>? BeforeOutgoingCall { get; set; } | ||
| public UiPath.Ipc.ContractCollection? Callbacks { get; set; } | ||
| public Microsoft.Extensions.Logging.ILogger? Logger { get; init; } | ||
| public UiPath.Ipc.ClientTransport Transport { get; init; } | ||
| public TProxy GetProxy<TProxy>() | ||
| where TProxy : class { } | ||
| } | ||
| public class IpcProxy : System.Reflection.DispatchProxy, System.IDisposable | ||
|
There was a problem hiding this comment. Why would we have this public? There was a problem hiding this comment. Unfortunately, the number of usages is non-zero, some of which are scarier than others, i.e.:
gets cast to We also have more legit usages around logging categories/filtering. There was a problem hiding this comment. for the event, we should be able to relocate the ConnectionClosed event on the IpcClient itself> anyway, I'd like to try to remove this, but if it's costly, please tell me to evaluate options. There was a problem hiding this comment. But it shows that some CoreIPC users want to do connection management. Either close the connection, or know when the connection was closed, and the current API doesn't offer that. Perhaps IpcClient/IpcServer should offer a way to manage that? Not sure how scary the usage is in Robot. It seems they just want to close the connection, and make sure it's closed. I think before the revamp they had only a method to know when the connection is closed, using IpcHelpers in a rather special way, having the event subscription in the builder. It would be a more obvious to have an event in the IpcClient/IpcServer. ) Disconnecting might be done perhaps with disposing the IpcServer/IpcClient ? I guess there are more options here, this is just one of them. |
||
| { | ||
| public IpcProxy() { } | ||
| public System.IO.Stream? Network { get; } | ||
| public event System.EventHandler ConnectionClosed; | ||
| public System.Threading.Tasks.ValueTask CloseConnection() { } | ||
| public void Dispose() { } | ||
| protected override object? Invoke(System.Reflection.MethodInfo? targetMethod, object?[]? args) { } | ||
| } | ||
| public sealed class IpcServer : UiPath.Ipc.IpcBase, System.IAsyncDisposable | ||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public IpcServer() { } | ||
| public UiPath.Ipc.ContractCollection Endpoints { get; init; } | ||
| public UiPath.Ipc.ServerTransport Transport { get; init; } | ||
| public System.Threading.Tasks.ValueTask DisposeAsync() { } | ||
| [System.Diagnostics.CodeAnalysis.MemberNotNull(new string[] { | ||
| "Transport", | ||
| "_accepter"})] | ||
| public void Start() { } | ||
| public System.Threading.Tasks.Task WaitForStart() { } | ||
|
There was a problem hiding this comment. Do we need WaitForStart anywhere? If we do, what about Start being async? There was a problem hiding this comment. As a CoreIpc server user, how can I know if a client connected or disconnected? It seems these are missing, no? Maybe not required by Robot, but seems pretty basic. |
||
| } | ||
| public class Message | ||
| { | ||
| public Message() { } | ||
| [Newtonsoft.Json.JsonIgnore] | ||
| public UiPath.Ipc.IClient Client { get; set; } | ||
| [Newtonsoft.Json.JsonIgnore] | ||
| public System.TimeSpan RequestTimeout { get; set; } | ||
| public TCallbackInterface GetCallback<TCallbackInterface>() | ||
|
There was a problem hiding this comment. 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. |
||
| where TCallbackInterface : class { } | ||
| public void ImpersonateClient(System.Action action) { } | ||
| } | ||
| public class Message<TPayload> : UiPath.Ipc.Message | ||
| { | ||
| public Message(TPayload payload) { } | ||
| public TPayload Payload { get; } | ||
| } | ||
| public class RemoteException : System.Exception | ||
| { | ||
| public RemoteException(UiPath.Ipc.Error error) { } | ||
| public UiPath.Ipc.RemoteException? InnerException { get; } | ||
| public override string StackTrace { get; } | ||
| public string Type { get; } | ||
| public bool Is<TException>() | ||
| where TException : System.Exception { } | ||
| public override string ToString() { } | ||
| } | ||
| public abstract class ServerTransport | ||
| { | ||
| public int ConcurrentAccepts { get; set; } | ||
| public byte MaxReceivedMessageSizeInMegabytes { get; set; } | ||
| } | ||
| } | ||
| namespace UiPath.Ipc.Transport.NamedPipe | ||
| { | ||
| public sealed class NamedPipeClientTransport : UiPath.Ipc.ClientTransport, System.IEquatable<UiPath.Ipc.Transport.NamedPipe.NamedPipeClientTransport> | ||
|
There was a problem hiding this comment. 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. |
||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public NamedPipeClientTransport() { } | ||
| public bool AllowImpersonation { get; init; } | ||
| public string PipeName { get; init; } | ||
| public string ServerName { get; init; } | ||
| public override string ToString() { } | ||
| } | ||
| public sealed class NamedPipeServerTransport : UiPath.Ipc.ServerTransport | ||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public NamedPipeServerTransport() { } | ||
| [Newtonsoft.Json.JsonIgnore] | ||
| public System.Action<System.IO.Pipes.PipeSecurity>? AccessControl { get; init; } | ||
| public string PipeName { get; init; } | ||
| public string ServerName { get; init; } | ||
| public override string ToString() { } | ||
| } | ||
| } | ||
| namespace UiPath.Ipc.Transport.Tcp | ||
| { | ||
| public sealed class TcpClientTransport : UiPath.Ipc.ClientTransport, System.IEquatable<UiPath.Ipc.Transport.Tcp.TcpClientTransport> | ||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public TcpClientTransport() { } | ||
| public System.Net.IPEndPoint EndPoint { get; init; } | ||
| public override string ToString() { } | ||
| } | ||
| public sealed class TcpServerTransport : UiPath.Ipc.ServerTransport | ||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public TcpServerTransport() { } | ||
| public System.Net.IPEndPoint EndPoint { get; init; } | ||
| public override string ToString() { } | ||
| } | ||
| } | ||
| namespace UiPath.Ipc.Transport.WebSocket | ||
| { | ||
| public sealed class WebSocketClientTransport : UiPath.Ipc.ClientTransport, System.IEquatable<UiPath.Ipc.Transport.WebSocket.WebSocketClientTransport> | ||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public WebSocketClientTransport() { } | ||
| public System.Uri Uri { get; init; } | ||
| public override string ToString() { } | ||
| } | ||
| public sealed class WebSocketServerTransport : UiPath.Ipc.ServerTransport | ||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public WebSocketServerTransport() { } | ||
| public System.Func<System.Threading.CancellationToken, System.Threading.Tasks.Task<System.Net.WebSockets.WebSocket>> Accept { get; init; } | ||
| public override string ToString() { } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
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.