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

remove loaded sessions when unneeded #52

Merged
merged 9 commits into from
Oct 16, 2023
Merged

Conversation

maxcountryman
Copy link
Owner

Here we implement a basic reference counting strategy to allow removal of loaded sessions.

Tracking loaded sessions in this way is necessary to support concurrent session access. For example, if we load from a store in order to provide a session as a request extension, this will load stale data. This happens when concurrently the same session is being modified. In other words, stale data can overwrite the session before its been persisted to the store.

To prevent this without losing the ability to concurrently operate over a session we manage shared memory via a map. When we load a session we also insert this session into the map of loaded sessions. Future requests will first try to read from the map before falling back to the store.

Problematically this map will grow indefinitely, since the only point we can positively remove a session from the map is when it's explicitly deleted or expired. However, since the purpose of this map is shared memory, we only need the session so long as it's being referenced.

This patch adds a simple wrapper around the session which includes a reference count. With some extra accounting, we are now able to safely remove the session from the map without an obvious data race. This ensures that the loaded session map does not grow unboundedly.

Closes #51.

Here we implement a basic reference counting strategy to allow removal
of loaded sessions.

Tracking loaded sessions in this way is necessary to support concurrent
session access. For example, if we load from a store in order to provide
a session as a request extension, this will load stale data. This
happens when concurrently the same session is being modified. In other
words, stale data can overwrite the session before its been persisted to
the store.

To prevent this without losing the ability to concurrently operate over
a session we manage shared memory via a map. When we load a session we
also insert this session into the map of loaded sessions. Future
requests will first try to read from the map before falling back to the
store.

Problematically this map will grow indefinitely, since the only point we
can positively remove a session from the map is when it's explicitly
deleted or expired. However, since the purpose of this map is shared
memory, we only need the session so long as it's being referenced.

This patch adds a simple wrapper around the session which includes a
reference count. With some extra accounting, we are now able to safely
remove the session from the map without an obvious data race. This
ensures that the loaded session map does not grow unboundedly.

Closes #51.
@maxcountryman
Copy link
Owner Author

There are likely better approaches here, so for future readers: if you have ideas regarding how we might improve this, please open a discussion or submit a PR. Taking a step back, the larger goal is to enable concurrent access to the session. So maybe we don't need a map to do that and instead can use a clever solution that leverages channels, etc.

@maxcountryman maxcountryman merged commit caa7868 into main Oct 16, 2023
13 checks passed
@maxcountryman maxcountryman deleted the loaded-sessions-gc branch October 16, 2023 00:15
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.

SessionManager infinitely caching sessions in loaded_sessions
1 participant