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

fix(rpc): make sure to run rpc request futures till completion #2237

Closed
wants to merge 2 commits into from

Conversation

mariocynicys
Copy link
Collaborator

when ran directly inside hyper's service function, it might get aborted mid-way when a client disconnects, leaving the future incomplete.

this is an issue as we might have some code that need to be executed atomically inside any part of the code base that could be triggered by an RPC request. if the client disconnects, such a function will abort on the next await call, leaving us with non-atomic state (e.g. partial update for a map but not its inverse).

when ran directly inside hyper's service function, it might get aborted mid-way when a client disconnects, leaving the future in complete.

this is an issue as we might have some code that need to be executed atomically inside any part of the code base that could be triggered by an RPC request. if the client disconnects, such a function will abort on the next await call, leaving us with non-atomic state (e.g. partial update for a map and its inverse).
@mariocynicys
Copy link
Collaborator Author

mariocynicys commented Oct 10, 2024

let me move this back to in progress and try to give it one more shot at finding if there is a config we can set to let hyper not shutdown our request resolver future when the client disconnects.

EDIT: Nope, there is no config option in hyper for that.

@mariocynicys mariocynicys added in progress Changes will be made from the author under review and removed under review in progress Changes will be made from the author labels Oct 10, 2024
@mariocynicys mariocynicys force-pushed the run-rpc-futures-till-completion branch from 28887f0 to 783980b Compare October 15, 2024 13:34
looks like this was only here to simulate a panic from the rpc end.
why you ask? i guess we will never know!
@mariocynicys mariocynicys force-pushed the run-rpc-futures-till-completion branch from 783980b to b95110e Compare October 15, 2024 15:11
Copy link
Member

@borngraced borngraced left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 🚀

@mariocynicys
Copy link
Collaborator Author

Looks like I mistakenly included this PR already in #1966 (5e38086).
Do we want to revert that change from dev now @shamardy? We can also keep this review and any needed changed would be applied here.

@shamardy
Copy link
Collaborator

Do we want to revert that change from dev now @shamardy? We can also keep this review and any needed changed would be applied here.

No need to revert the change, I will include an entry about it in the changelog #2240 (comment). I will check this PR and will close it if everything is fine. As I already reviewed #1966, there should be no issues or any changes required I think.

@shamardy
Copy link
Collaborator

Duplicate as it was handled in #1966

@shamardy shamardy closed this Oct 30, 2024
@mariocynicys mariocynicys deleted the run-rpc-futures-till-completion branch November 8, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants