fix session saving and loading data race #35
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes a bug where session data could be lost if a session is loaded from the store while it's also being saved back to the store. Because sessions are loaded and stored in memory at the beginning of a request, it was previously possible for a session's inner value to be correctly altered but for that modification to be lost between the moment the session is saved to the store and when it's loaded. In other words, the data loaded would not reflect the in-progress update and instead be stale. To address this we could hold a lock for the duration of the request. However, this imposes a performance penalty we would like to avoid. Alternatively we can manage a registry of sessions, a la tower-cookie's management of `CookieJar`: with this, we load a session first from the service's cache of sessions and fallback to the store only when the session is not present. Currently sessions are managed like this via DashMap. However, alternatively caching facilities, such as Moka, might prove to be a better fit. Further, the current implementation does not bound the total number of cached sessions--this should be addressed in future updates. At present, this means memory will continue to grow, albeit typically at a very small rate; this is a memory leak! Because sessions are safely written to and read from this cache, we prevent stale reads and thus a data race which could lead to data loss in highly concurrent environments, especially where multiple requests are made using the same session cookie simultaneously. Note that `replace_if_equal` is still required to prevent data races in application code.
Codecov Report
@@ Coverage Diff @@
## main #35 +/- ##
==========================================
+ Coverage 78.55% 78.73% +0.18%
==========================================
Files 13 13
Lines 415 428 +13
==========================================
+ Hits 326 337 +11
- Misses 89 91 +2
|
maxcountryman
added a commit
that referenced
this pull request
Oct 9, 2023
This is a temporary fix for a data race that occurs when a session is saved and loaded at the same time. The result of this race is that stale data can be loaded into memory, resulting in potential data loss where multiple requests using the same session are issued simultaneously. Here we address this by holding a lock for the duration of the request. This solution is only temporary, as the underlying session implementation can instead be modified to ensure safe access between the save and load await points. This supercedes #35.
maxcountryman
added a commit
that referenced
this pull request
Oct 9, 2023
* fix session saving and loading data race This is a temporary fix for a data race that occurs when a session is saved and loaded at the same time. The result of this race is that stale data can be loaded into memory, resulting in potential data loss where multiple requests using the same session are issued simultaneously. Here we address this by holding a lock for the duration of the request. This solution is only temporary, as the underlying session implementation can instead be modified to ensure safe access between the save and load await points. This supercedes #35.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a bug where session data could be lost if a session is loaded from the store while it's also being saved back to the store. Because sessions are loaded and stored in memory at the beginning of a request, it was previously possible for a session's inner value to be correctly altered but for that modification to be lost between the moment the session is saved to the store and when it's loaded. In other words, the data loaded would not reflect the in-progress update and instead be stale.
To address this we could hold a lock for the duration of the request. However, this imposes a performance penalty we would like to avoid. Alternatively we can manage a registry of sessions, a la tower-cookie's management of
CookieJar: with this, we load a session first from the service's cache of sessions and fallback to the store only when the session is not present.Currently sessions are managed like this via DashMap. However, alternatively caching facilities, such as Moka, might prove to be a better fit. Further, the current implementation does not bound the total number of cached sessions--this should be addressed in future updates. At present, this means memory will continue to grow, albeit typically at a very small rate; this is a memory leak!
Because sessions are safely written to and read from this cache, we prevent stale reads and thus a data race which could lead to data loss in highly concurrent environments, especially where multiple requests are made using the same session cookie simultaneously.
Note that
replace_if_equalis still required to prevent data races in application code.