Replies: 5 comments 7 replies
-
This seems reasonable to me. It's a bit unfortunate that this is a breaking change that requires store implementations to handle a new variant. |
Beta Was this translation helpful? Give feedback.
-
What if we do it in two steps? The next minor release of tower-sessions could just include the new enum variant (this would be a major release for the tower-sessions-core), but not change the error handling by default (thus still produce an internal server error). This would be compilable for all store implementations that don't match on the Error enum variants. I can't think of a valid use case for that so I think it might be reasonable to do that. Also, stores would be allowed to start returning the new error variant. If a store does that and is used with the older version, they would not get retries and would still get an internal server error. Apps could also opt into the retry by constructing the ServiceManager with a different constructor (and a corresponding new constructor in the ServiceManagerLayer that allows opting into the retry functionality. Something like this:
For the major version, we could change the ServieManager and ServiceManagerLayers to use builders for construction and have a builder method that turns on the functionality, like this:
Alternatively, we could move all the builder type methods in SessionManagerLayer to the builder. Or...we could technically add a method to the SessionConfig to enable retries and add a method to the SessionManager to enable the retries and make the new constructor or add a new to SessionManagerLayer to enable the retries. I think this is the ugliest solution because it's not clear to me that the config for the retries should live in the SessionConfig. I get that adding the enum variant by itself is technically a breaking change, but this seems pretty low risk for causing issues with SessionStores. Obviously, we could also just implement whatever design with a new major version for tower-sessions. If we have to do that, do you have a timeline for the next major release? Do you have any opinions on these options? |
Beta Was this translation helpful? Give feedback.
-
Also, if we must do a major revision, maybe we can redesign the Error a bit to hide some of the details as indicated in the docs for the thiserror crate here. Maybe we should do something like this:
|
Beta Was this translation helpful? Give feedback.
-
I'm thinking a bit more here and (apologies I'm not at the computer so I may be misremembering) isn't it the case that a preexisting ID would not result in an error in the first place? Consider that we are either saving, loading, or deleting a record--none of these actions encode the fact that a record should not already exist. What I believe this means is a collision wouldn't result in an error, it would just operate over the associated session. Maybe I've missed something here so again please jump in and correct me. If I'm right tho this is a serious problem (albeit one still as unlikely as we've previously pointed out) because it means a collision could give you someone else's session (!). To address this, we might need a new session store method, like "create", to ensure that we can identify when a session should not already exist. |
Beta Was this translation helpful? Give feedback.
-
I think having a different method is important here to make it easier to
understand. I'm actually doing some experimentation to see what this could
look like. I have some ideas about incrementally changing so that it's not
an immediate major version bump.
Wren Turkal
You're more amazing than you think! ymatyt.com
…On Sun, Mar 10, 2024, 06:44 Max Countryman ***@***.***> wrote:
I can think of two immediate ways we might address this:
1. We add some kind of context to or around the record type to
indicate if it's meant to be created and not already existing or
2. We follow along with the Django implementation and introduce a
create method to the SessionStore trait; this would be called before
saving when the session ID is None.
Because the record is currently directly serializable I'm not really sure
where that context would be stored on the record itself. Maybe rather than
encoding that directly on the type it could instead be a parameter, like
should_create, passed to save.
I don't really have a strong preference between the two (and maybe there
better options I'm not immediately thinking of), both are breaking changes
that require stores to update in some way. Even so, the failure mode is so
catastrophic I feel like we must account for this.
—
Reply to this email directly, view it on GitHub
<#169 (reply in thread)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADFT76RKCS3WYO4BRULIDDYXRWVZAVCNFSM6AAAAABEOBISZSVHI2DSMVQWIX3LMV43SRDJONRXK43TNFXW4Q3PNVWWK3TUHM4DOMZVG42DI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***
.com>
|
Beta Was this translation helpful? Give feedback.
-
I haven't found any collision specific detection when creating a new session record. I do see that a store to the session store can fail, which results in an internal server error response. This is probably reasonable since a 128 bit random value like Id shouldn't collide very often.
However, I was wondering if we could have a new session_store::Error type that indicates a duplicate id was attempted to be stored. Then, maybe react to that error with a retry on generating a new session id, ideally logging whenever a collision occurs. This overhead would only be incurred when creating a new session Id and would prevent an unfortunate internal server error.
Beta Was this translation helpful? Give feedback.
All reactions