feat: improve session extend performance #3948
Merged
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 patch improves the performance for extending session lifespans. Lifespan extension is tricky as it usually is part of the middleware of Ory Kratos consumers. As such, it is prone to transaction contention when we read and write to the same session row at the same time (and potentially multiple times). To address this, we: 1. Introduce a locking mechanism on the row to reduce transaction contention; 2. Add a new feature flag which toggles returning 204 no content instead of 200 + session. Be aware that all reads on the session table will have to wait for the transaction to commit before they return a value. This may cause long(er) response times on `/session/whoami` for sessions that are being extended at the same time.
|
You can play around yourself with table locking in this guide using |
jonas-jonas
previously approved these changes
Jun 6, 2024
alnr
reviewed
Jun 6, 2024
persistence/sql/persister_session.go
Outdated
| return sqlcon.HandleError(err) | ||
| } | ||
|
|
||
| trace.SpanFromContext(ctx).AddEvent(events.NewSessionLifespanExtended(ctx, s.ID, s.IdentityID, s.ExpiresAt)) |
There was a problem hiding this comment.
This must be moved outside of the transaction into a deferred function call in order not to send duplicate events when the TX automatically retries (see UpsertSession).
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3948 +/- ##
==========================================
+ Coverage 78.10% 78.18% +0.08%
==========================================
Files 362 362
Lines 25329 25433 +104
==========================================
+ Hits 19783 19885 +102
Misses 4028 4028
- Partials 1518 1520 +2 ☔ View full report in Codecov by Sentry. |
fenech
pushed a commit
to nbycomp/kratos
that referenced
this pull request
Jun 14, 2024
This patch improves the performance for extending session lifespans. Lifespan extension is tricky as it is often part of the middleware of Ory Kratos consumers. As such, it is prone to transaction contention when we read and write to the same session row at the same time (and potentially multiple times). To address this, we: 1. Introduce a locking mechanism on the row to reduce transaction contention; 2. Add a new feature flag that toggles returning 204 no content instead of 200 + session. Be aware that all reads on the session table will have to wait for the transaction to commit before they return a value. This may cause long(er) response times on `/session/whoami` for sessions that are being extended at the same time. BREAKING CHANGES: Going forward, the `/admin/session/.../extend` endpoint will return 204 no content for new Ory Network projects. We will deprecate returning 200 + session body in the future.
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 patch improves the performance for extending session lifespans. Lifespan extension is tricky as it usually is part of the middleware of Ory Kratos consumers. As such, it is prone to transaction contention when we read and write to the same session row at the same time (and potentially multiple times).
To address this, we:
Be aware that all reads on the session table will have to wait for the transaction to commit before they return a value. This may cause long(er) response times on
/session/whoamifor sessions that are being extended at the same time.BREAKING CHANGES: Going forward, the
/admin/session/.../extendendpoint will return 204 no content for new Ory Network projects. We will deprecate returning 200 + session body in the future.Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments