theRainbird / CoreRemoting

RPC library with classic .NET Remoting flavour
MIT License
63 stars 23 forks source link

Async API issues #84

Open yallie opened 2 days ago

yallie commented 2 days ago

Describe the bug It's not exactly a bug, but rather a technical debt I'm asking to discuss. Hope I'm not missing something obvious...

To Reproduce Modern network libraries are asynchronous, but CoreRemoting channel API is synchronous.

Expected behavior Network protocol implementations provide SendAsync, ListenAsync and DisposeAsync methods rather than Send, Listen and Dispose. Establishing the connection requires network roundtrip which can take time. Sending data packets can be time-consuming for large payloads. Disposing of the channel may involve sending data to remote machine to signal the end of session. And so on. Performing these time-consuming operations on a main application thread should be avoided.

Additional notes If channel API is refactored to be asynchronous, it's still can be used for legacy synchronous libraries. For instance, void methods can return Task.CompletedTask, and so on.

I'm proposing something like this:

public interface IClientChannel : IDisposableAsync
{
    Task Init(IRemotingClient client);

    Task Connect();

    Task Disconnect();

    Task<bool> IsConnected { get; } // not sure about this :)

    IRawMessageTransport RawMessageTransport { get; }

    event Action Disconnected;
}

public interface IServerChannel : IDisposableAsync
{
    Task Init(IRemotingServer server);

    Task StartListening();

    Task StopListening();

    Task<bool> IsListening { get; } // not sure again
}

public interface IRawMessageTransport
{
    Task<bool> SendMessage(byte[] rawMessage);

    event Action<byte[]> ReceiveMessage;

    event Action<string, Exception> ErrorOccured;

    NetworkException LastException { get; set; }
}
sancheolz commented 2 days ago

I would like to add that the synchronization primitives CountdownEvent, ManualResetEvent and others should be replaced with their asynchronous counterparts. Otherwise, the thread pool will be exhausted at high load. If we using asyn/await and tasks, then primitives should be awaitable.

yallie commented 2 days ago

I would like to add that the synchronization primitives CountdownEvent, ManualResetEvent and others should be replaced with their asynchronous counterparts. Otherwise, the thread pool will be exhausted at high load. If we using asyn/await and tasks, then primitives should be awaitable.

Agreed! 👍