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

EvictingMap manages state between await boundaries #1156

Open
allada opened this issue Jul 14, 2024 · 3 comments · May be fixed by #1300
Open

EvictingMap manages state between await boundaries #1156

allada opened this issue Jul 14, 2024 · 3 comments · May be fixed by #1300

Comments

@allada
Copy link
Member

allada commented Jul 14, 2024

If you look at the following code:

if let Some(old_item) = state.put(key, eviction_item).await {

You'll notice that we change the state of struct after this await call. This is quite dangerous because our future can die at any point .await is called. This is quite tricky to manage and easy to overlook. In reality, what we want to do is to prevent this future (and others similar) to not be able to be dropped in the middle of anywhere in the future (ie: always complete future). The way this is normally done is via tokio::spawn, but this is a very heavy handed way to handle this.

What I believe would be a good solution is to make a future wrapper that holds the state of if the inner future completed. If it was not completed, implement a drop function that will move the future to a background spawn and ignore the results. This in theory should give us the best of both worlds. We'll be able to keep the future on the stack, but in the rare event the future gets dropped in the middle of a critical state managed future, it'll still execute but as a background spawn.

@leodziki
Copy link

I am going to work on this issue.
cc: @allada, @MarcusSorealheis

@MarcusSorealheis
Copy link
Collaborator

SGTM

@leodziki leodziki linked a pull request Aug 31, 2024 that will close this issue
5 tasks
@leodziki
Copy link

Please review the implementation of DropGuard and consider how this behavioral change might impact any existing workflows.
cc: @allada, @MarcusSorealheis

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 a pull request may close this issue.

3 participants