eclipse-uprotocol / up-cpp

uProtocol Language Specific Library for C++
Apache License 2.0
18 stars 25 forks source link

Replace futures in RpcClient with callbacks #108

Closed gregmedd closed 4 months ago

gregmedd commented 5 months ago

@sanjok-itin has pointed out that waiting on std::futures does not allow for a shutdown path. Either threads waiting on futures need to wake periodically to check if they need to stop waiting, or we need to wake them by breaking the promises.

We should look into implementing some sort of shutdown interface so that we can break promises and block any new calls to the transport APIs.

Alternatively, returning some sort of other waitable object that can be signaled from both ends of the connection could be used, or we could return something that can be waited on as a group (e.g. using poll() or select() or epoll())

sanjok-itin commented 5 months ago

RpcClient::invokeMethod() returns std::future, which does not provide the really asynchronous API.

The user has a few options to deal with the response and all of them are bad:

  1. to block on wait() method - synchronous behavior.
  2. to use wait_for() method periodically. the short periods affect the CPU usage due to the context switch. the long periods increase the latency.
  3. to have the dedicated thread for every concurrent RPC request and to use synchronous behavior. requires to know the maximal number of concurrent requests on the process initialization in order to pre-open the threads or to open the threads during the run-time. In any case it requires more resources than might be needed.

I understood that the callback variation of invokeMethod() was removed because the callback is invoked in the up-cpp context.

I have another suggestion: To add an optional parameter of std::counting_semaphore* to invokeMethod(). If the provided semaphore is not null, up-cpp will call to release() right after it called to promise::set_value(). It allows to the user to open a single thread that will sleep on the this semaphore, and to check all the response futures.

Pros:

  1. CPU is not loaded due to periodic checks
  2. the bigger number of concurrent requests does not require additional threads
  3. the latency is minimal
  4. there are no applicative callbacks called in up-cpp context
  5. the user's code, which handles the termination, can wakeup the sleeping thread using release() method

Cons:

  1. it requires c++20
gregmedd commented 5 months ago

Having thought on this more, UTransport already has a safe shutdown process for its components. Only RpcClient would need to add this interface.

I've also just been informed that there is active discussion on changing the requirements of the RpcClient API in up-spec, so this issue will be on hold until that is decided.

gregmedd commented 5 months ago

@sanjok-itin - I've got confirmation that the spec is being relaxed in this area and that callbacks instead of futures would be acceptable in the API. I'm going to investigate and make sure we can support that, then put it on our roadmap. If we can switch to callbacks, then we might not need the shutdown interface at all (depending on the RPC client design).