search

Derecho-Project / derecho

The main code repository for the Derecho project.
BSD 3-Clause "New" or "Revised" License
187 stars 47 forks source link

Client callbacks #226

Closed songweijia closed 2 years ago

songweijia commented 2 years ago

Hi Edward,

When I try to debug the issue you mentioned, I found there is a couple of issues in the code that need to be addressed first. 1) The REGISTER_RPC_FUNCTION only supports two parameters for either grouped P2P_TARGETS() and ORDERED_TARGETS(). I rewrote this part so that the user code can put any number (actually less than 99) of grouped P2P_TARGETS and ORDERED_TARGETS. This allows the REGISTER_RPC_FUNCTION_WITH_NOTIFICATION macro to work in an elegant way. 2) TODO: the lambda registry in NotificationSupport needs to be protected by a mutex since it will be accessed from multiple threads. 3) TODO: the lambda registry in NotificationSupport needs an unregister() API.

I'm working on those issues and eventually along with the bug you mentioned. I used a fork based on your latest client_callbacks branch. You don't have to merge this pull request now. I just want to update you about the changes in case you want to know what I'm doing. Also, please let me know if you don't agree some of those design.

etremel commented 2 years ago

Thanks for working on this. I had already noticed the problem with the REGISTER_RPC_FUNCTION_WITH_NOTIFICATION macro and just didn't have time to fix it, because it would require a lot of changes to let REGISTER_RPC_FUNCTIONS accept more than 2 arguments. And yes, I agree that the NotificationSupport class could be a little more robust.

songweijia commented 2 years ago

@etremel I think I've fixed what I mentioned in my previous comment. However, I couldn't reproduce the 'external client stops sending ack' issue you told me on slack. I've shared the tester I use with you. We should find a time to check it together.

songweijia commented 2 years ago

@etremel, you can merge this version. I will delete my fork now. Then we can come up with a forever fix for issue #217 based on the workaround we have this afternoon.