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,264 @@ | ||
| [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.Tests")] | ||
| [assembly: System.Runtime.CompilerServices.InternalsVisibleTo("UiPath.Ipc.TV")] | ||
| [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 sealed class ClientConfig : UiPath.Ipc.EndpointConfig, System.IEquatable<UiPath.Ipc.ClientConfig> | ||
| { | ||
| public ClientConfig() { } | ||
| public string DebugName { get; set; } | ||
| public UiPath.Ipc.ISerializer? Serializer { get; set; } | ||
| public System.Func<UiPath.Ipc.CallInfo, System.Threading.CancellationToken, System.Threading.Tasks.Task>? BeforeCall { get; init; } | ||
| public System.Func<System.Threading.CancellationToken, System.Threading.Tasks.Task>? BeforeConnect { get; init; } | ||
| public UiPath.Ipc.EndpointCollection? Callbacks { get; init; } | ||
| public Microsoft.Extensions.Logging.ILogger? Logger { get; init; } | ||
| public System.Threading.Tasks.TaskScheduler? Scheduler { get; init; } | ||
| public System.IServiceProvider? ServiceProvider { get; init; } | ||
| } | ||
| public abstract class ClientTransport : System.IEquatable<UiPath.Ipc.ClientTransport> | ||
vuplea marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| protected ClientTransport() { } | ||
| public abstract UiPath.Ipc.IClientState CreateState(); | ||
| public abstract void Validate(); | ||
|
There was a problem hiding this comment. maybe we should think if we really need the Validate feature? |
||
| } | ||
| public class EndpointCollection : System.Collections.Generic.IEnumerable<UiPath.Ipc.EndpointSettings>, System.Collections.IEnumerable | ||
vuplea marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| public EndpointCollection() { } | ||
| public void Add(System.Type type) { } | ||
| public void Add(UiPath.Ipc.EndpointSettings endpointSettings) { } | ||
| public void Add(System.Type contractType, object? instance) { } | ||
| public System.Collections.Generic.IEnumerator<UiPath.Ipc.EndpointSettings> GetEnumerator() { } | ||
vuplea marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| public abstract class EndpointConfig : System.IEquatable<UiPath.Ipc.EndpointConfig> | ||
| { | ||
| protected EndpointConfig() { } | ||
| public System.TimeSpan RequestTimeout { get; init; } | ||
| } | ||
| [System.Serializable] | ||
|
There was a problem hiding this comment. ISerializable is obsolete: dotnet/docs#34893 |
||
| public sealed class EndpointNotFoundException : System.ArgumentOutOfRangeException | ||
|
There was a problem hiding this comment. 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 |
||
| { | ||
| public EndpointNotFoundException(string paramName, string serverDebugName, string endpointName) { } | ||
| public string EndpointName { get; } | ||
| public string ServerDebugName { get; } | ||
vuplea marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| public class EndpointSettings : System.IEquatable<UiPath.Ipc.EndpointSettings> | ||
| { | ||
| public EndpointSettings(System.Type contractType, System.IServiceProvider serviceProvider) { } | ||
| public EndpointSettings(System.Type contractType, object? serviceInstance = null) { } | ||
| public System.Func<UiPath.Ipc.CallInfo, System.Threading.CancellationToken, System.Threading.Tasks.Task>? BeforeCall { get; set; } | ||
|
There was a problem hiding this comment. we have to decide where we put the recurring configurations. We want a single place for them. we have to keep it as simple, this was one of our goals.
There was a problem hiding this comment. We can have a single constructor (Type, Func?) I think for advanced needs. |
||
| public System.Type ContractType { get; } | ||
| public System.Threading.Tasks.TaskScheduler? Scheduler { get; set; } | ||
| public object? ServiceInstance { get; } | ||
| public System.IServiceProvider? ServiceProvider { get; } | ||
| public void Validate() { } | ||
|
There was a problem hiding this comment. I see you've added ServiceProvider and scheduler to IpcBase - so we'd be good to remove them from here, right? |
||
| public virtual UiPath.Ipc.EndpointSettings WithServiceProvider(System.IServiceProvider? serviceProvider) { } | ||
| } | ||
| public sealed class EndpointSettings<TContract> : UiPath.Ipc.EndpointSettings, System.IEquatable<UiPath.Ipc.EndpointSettings<TContract>> | ||
| where TContract : class | ||
| { | ||
| public EndpointSettings(System.IServiceProvider serviceProvider) { } | ||
| public EndpointSettings(TContract? serviceInstance = null) { } | ||
| public override UiPath.Ipc.EndpointSettings WithServiceProvider(System.IServiceProvider? serviceProvider) { } | ||
| } | ||
| [System.Serializable] | ||
| 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 interface IClientState : System.IDisposable | ||
|
There was a problem hiding this comment. why not directly these in ClientTransport? There was a problem hiding this comment. Looking into it, maybe the tensions between parts disappeared, and I hadn't checked it. I hope so because, yeah, it's pretty complex. |
||
| { | ||
| System.IO.Stream? Network { get; } | ||
| System.Threading.Tasks.ValueTask Connect(UiPath.Ipc.IpcClient client, System.Threading.CancellationToken ct); | ||
| bool IsConnected(); | ||
| } | ||
| 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) { } | ||
| 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 interface ISerializer | ||
|
There was a problem hiding this comment. Realistically, experience (msgpack, System.Text.Json) showed us a generic abstraction over serialization is not that plausible. |
||
| { | ||
| object? Deserialize(string json, System.Type type); | ||
| System.Threading.Tasks.ValueTask<T?> DeserializeAsync<T>(System.IO.Stream json, Microsoft.Extensions.Logging.ILogger? logger); | ||
| string Serialize(object? obj); | ||
| void Serialize(object? obj, System.IO.Stream stream); | ||
| } | ||
| public sealed class IpcClient | ||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public IpcClient() { } | ||
| public UiPath.Ipc.ClientConfig Config { 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. |
||
| { | ||
| 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 : 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.EndpointCollection Endpoints { get; init; } | ||
| public System.Collections.Generic.IReadOnlyList<UiPath.Ipc.ListenerConfig> Listeners { get; init; } | ||
|
There was a problem hiding this comment. I think we can improve the entity model to have better matching between client and server, and generally to simplify it.
I'd propose this model: |
||
| public System.Threading.Tasks.TaskScheduler? Scheduler { get; init; } | ||
| public System.IServiceProvider ServiceProvider { get; init; } | ||
| public System.Threading.Tasks.ValueTask DisposeAsync() { } | ||
| public void Start() { } | ||
| public System.Threading.Tasks.Task WaitForStart() { } | ||
| public System.Threading.Tasks.Task WaitForStop() { } | ||
|
There was a problem hiding this comment. what;s the differentce between WaitForStop() and DisposeAsync()? |
||
| } | ||
| public abstract class ListenerConfig : UiPath.Ipc.EndpointConfig, System.IEquatable<UiPath.Ipc.ListenerConfig> | ||
| { | ||
| protected ListenerConfig() { } | ||
| public System.Security.Cryptography.X509Certificates.X509Certificate? Certificate { get; init; } | ||
|
There was a problem hiding this comment. was this in CoreIpc? There was a problem hiding this comment. 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. |
||
| public int ConcurrentAccepts { get; init; } | ||
| public byte MaxReceivedMessageSizeInMegabytes { get; init; } | ||
| } | ||
| 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; } | ||
| } | ||
| [System.Serializable] | ||
| 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() { } | ||
| } | ||
| } | ||
| namespace UiPath.Ipc.Extensibility | ||
| { | ||
| public interface IListenerConfig<TSelf, TListenerState, TConnectionState> | ||
|
There was a problem hiding this comment. IListenerConfig vs ListenerConfig not obvious. Too many generics, one of the problems we wanted to avoid. There was a problem hiding this comment. 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. |
||
| where TSelf : UiPath.Ipc.ListenerConfig, UiPath.Ipc.Extensibility.IListenerConfig<TSelf, TListenerState, TConnectionState> | ||
| where TListenerState : System.IAsyncDisposable | ||
| { | ||
| System.Threading.Tasks.ValueTask<System.IO.Stream> AwaitConnection(TListenerState listenerState, TConnectionState connectionState, System.Threading.CancellationToken ct); | ||
| TConnectionState CreateConnectionState(UiPath.Ipc.IpcServer server, TListenerState listenerState); | ||
| TListenerState CreateListenerState(UiPath.Ipc.IpcServer server); | ||
| System.Collections.Generic.IEnumerable<string> Validate(); | ||
| } | ||
| } | ||
| namespace UiPath.Ipc.Transport.NamedPipe | ||
| { | ||
| public sealed class NamedPipeListener : UiPath.Ipc.ListenerConfig, System.IEquatable<UiPath.Ipc.Transport.NamedPipe.NamedPipeListener>, UiPath.Ipc.Extensibility.IListenerConfig<UiPath.Ipc.Transport.NamedPipe.NamedPipeListener, UiPath.Ipc.Transport.NamedPipe.NamedPipeListenerState, UiPath.Ipc.Transport.NamedPipe.NamedPipeServerConnectionState> | ||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public NamedPipeListener() { } | ||
| [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() { } | ||
| } | ||
| public sealed class NamedPipeTransport : UiPath.Ipc.ClientTransport, System.IEquatable<UiPath.Ipc.Transport.NamedPipe.NamedPipeTransport> | ||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public NamedPipeTransport() { } | ||
| public bool AllowImpersonation { get; init; } | ||
| public string PipeName { get; init; } | ||
| public string ServerName { get; init; } | ||
| public override UiPath.Ipc.IClientState CreateState() { } | ||
| public override string ToString() { } | ||
| public override void Validate() { } | ||
| } | ||
| } | ||
| namespace UiPath.Ipc.Transport.Tcp | ||
| { | ||
| public sealed class TcpListener : UiPath.Ipc.ListenerConfig, System.IEquatable<UiPath.Ipc.Transport.Tcp.TcpListener>, UiPath.Ipc.Extensibility.IListenerConfig<UiPath.Ipc.Transport.Tcp.TcpListener, UiPath.Ipc.Transport.Tcp.TcpListenerState, UiPath.Ipc.Transport.Tcp.TcpServerConnectionState> | ||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public TcpListener() { } | ||
| public System.Net.IPEndPoint EndPoint { get; init; } | ||
| public override string ToString() { } | ||
| } | ||
| public sealed class TcpTransport : UiPath.Ipc.ClientTransport, System.IEquatable<UiPath.Ipc.Transport.Tcp.TcpTransport> | ||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public TcpTransport() { } | ||
| public System.Net.IPEndPoint EndPoint { get; init; } | ||
| public override UiPath.Ipc.IClientState CreateState() { } | ||
| public override string ToString() { } | ||
| public override void Validate() { } | ||
| } | ||
| } | ||
| namespace UiPath.Ipc.Transport.WebSocket | ||
| { | ||
| public sealed class WebSocketListener : UiPath.Ipc.ListenerConfig, System.IEquatable<UiPath.Ipc.Transport.WebSocket.WebSocketListener>, UiPath.Ipc.Extensibility.IListenerConfig<UiPath.Ipc.Transport.WebSocket.WebSocketListener, UiPath.Ipc.Transport.WebSocket.WebSocketListenerState, UiPath.Ipc.Transport.WebSocket.WebSocketServerConnectionState> | ||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public WebSocketListener() { } | ||
| public System.Func<System.Threading.CancellationToken, System.Threading.Tasks.Task<System.Net.WebSockets.WebSocket>> Accept { get; init; } | ||
| public override string ToString() { } | ||
| } | ||
| public sealed class WebSocketTransport : UiPath.Ipc.ClientTransport, System.IEquatable<UiPath.Ipc.Transport.WebSocket.WebSocketTransport> | ||
| { | ||
| [System.Obsolete("Constructors of types with required members are not supported in this version of " + | ||
| "your compiler.", true)] | ||
| public WebSocketTransport() { } | ||
| public System.Uri Uri { get; init; } | ||
| public override UiPath.Ipc.IClientState CreateState() { } | ||
| public override string ToString() { } | ||
| public override void Validate() { } | ||
| } | ||
| } | ||
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.
not clear what DebugName does.
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.
It's a CoreIpc-traditional tagging mechanism for reading logs and understanding who's who.
I didn't like how
Namesounded (it seemed more functional than it actually was), so I prefixed it withDebug.On
support/v21.10the public ones were ListenerSettings.Name and Connection.Name, both of which are public, and which spin off intointernals(i.e.ServiceClientinherits theNamefrom it's associatedListenerif it's of the callback sort) and all end up in logs, from hundreds of places, i.e: this