Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace futures in RpcClient with callbacks #108

Closed
gregmedd opened this issue May 20, 2024 · 3 comments · Fixed by #155
Closed

Replace futures in RpcClient with callbacks #108

gregmedd opened this issue May 20, 2024 · 3 comments · Fixed by #155
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@gregmedd
Copy link
Contributor

@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())

@gregmedd gregmedd added the enhancement New feature or request label May 21, 2024
@sanjok-itin
Copy link

sanjok-itin commented May 21, 2024

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 gregmedd changed the title Add interface to UTransport to signal a shutdown Add interface to RpcClient to signal a shutdown May 21, 2024
@gregmedd
Copy link
Contributor Author

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 gregmedd self-assigned this May 21, 2024
@gregmedd gregmedd added this to the alpha.2 milestone May 21, 2024
@gregmedd
Copy link
Contributor Author

@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).

@gregmedd gregmedd changed the title Add interface to RpcClient to signal a shutdown Replace futures in RpcClient with callbacks May 31, 2024
@gregmedd gregmedd linked a pull request Jun 8, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
2 participants