Skip to content

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

Closed
mariocynicys wants to merge 2 commits intodevfrom
run-rpc-futures-till-completion
Closed

fix(rpc): make sure to run rpc request futures till completion#2237
mariocynicys wants to merge 2 commits intodevfrom
run-rpc-futures-till-completion

Conversation

@mariocynicys
Copy link
Copy Markdown
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
Copy Markdown
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.

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
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants