Extend ISession interface to support session storage state explicitly (Pyramid 2.0 idea)
a3kov opened this issue · 7 comments
Some other frameworks and languages have session interfaces with more explicit state.
Example, they have 2 additional methods such as open() and close()
It serves important purposes:
- Implementation knows explicit enter and exit points. In Pyramid I guess entry point is session constructor/factory and exit..undefined ? Because if my db session storage depends on pyramid_tm, and the transaction has been committed prematurely, session API is supposed to work but the session storage is unavailable...how it should be handled is unclear because the API does not have an opinion on this.
- As a result of (1) implementation can lock and unlock at these points to provide concurrency control and to avoid race conditions.
- It has state - atleast open or closed. Obviously you can't store something in a closed session so the API can provide meaningful exceptions and/or error handling
In general I'm comfortable saying that sessions should not assume to be accessible after the EXCVIEW tween has executed. This covers closing the session in either response callbacks or pyramid_tm while ensuring they can be mutated by exception views.
The ISession API is implicit by design as you mentioned and I do not see a benefit to changing that. There is absolutely no goal to support opening and closing multiple times in the lifecycle of a request which should null out the need for open and close. open is irrelevant here because the factory is functionally equivalent. Defining a close would also require adding an is_open API. The close is acceptable as being implicit so long as we give some minor guarantees about when conforming ISession implementations should do their cleanup - which I enumerated above.
One final note... if you don't want people mutating the session after it has done its cleanup you can handle this already by nulling out the session on the request so any later attempts to change it will fail loudly and helpfully. I'd accept a PR to improve this on the default sessions if that's an issue (I haven't looked).
One advantage that I can give example of, would be resource management. Session as a shared resource with concurrent control needs locking. You can only avoid locking if you don't care about data lost in sessions and other side effects of race conditions. Most people do care about these issues - even if they don't care about locking and don't understand the importance of it, they still don't like lost data.
So with explicit interface you also get explicit resource management. For example, if the request does not intend to work with session anymore, it may explicitly close it, and the locks will be released, improving concurrency for other requests. With current interface the underlying implementation can't decide if its the right time to release it's own resources or not, because locking/unlocking can be costly also and multiple such operations could be costly.
This is an artifact of the exact backend you're using for your session. We do not make any assumptions here and so adding an api for it does not make sense. You are absolutely welcome to add these APIs to your session and document them for people but I don't know why we would implement those APIs for our cookie-based sessions for example. I get the argument and appreciate it but I'm -1 on the change to the core interface as Pyramid wants to remain agnostic about such opinions. I'm happy that the interface does not prevent you from doing this in your app if it needs it.
If it helps to understand my decision to close this so quickly - we've been through some serious bikeshedding in this area before with requests to add an "id" to the session in order to generally track the server-side storage id. My argument here is consistent with the conclusions in rejecting the session id API so you're welcome to be upset about it but I think it won't sway the decision. :-)
You have a bad habit of making too fast decisions, Michael, without trying to understand the problem
First, you are wrong on this:
This is an artifact of the exact backend you're using for your session.
All backend cookie storages need it. ALL. That is due to the nature of session - shared resource with concurrent access.
If some backend does not do it, it doesn't mean they solve concurrency issues magically. They just ignore it. Ignorance is a bliss, right ?
Then, I'm using database backend. The database may, or may not require explicit locking from the application. But you are too fast on judging me in this case - I don't need explicit locking because I depend on the database to do all the locks automatically - thankfully modern SQL DBs are smart about serialization conflicts. I just care about other people who use backends which still have to do explicit locking.
I don't know why we would implement those APIs for our cookie-based sessions for example
Cookie-based solution are doomed to have race conditions. The only difference is the races happen inside the browsers. If you use cookie based storage you don't magically solve race conditions, you only pass the problem to the client side.
So your cookie-based session impl would have empty open() and close()
Big deal ? But for other backend-based solution, it would greatly improve the situation.
We do not make any assumptions here and so adding an api for it does not make sense.
Well you can try to be minimalist in this case and ignore the problem of concurrency. But the problem will not go away, and it would also mean that your API is not enough to describe the session. Seriously, I have a feeling that I have to prove that 2x2=4
You can go and check other frameworks and languages and most of them have a open/close API or similar. Many smart people worked on this and I did not invent the whole thing.
It is not the framework's place to make a decision on how to handle two concurrent connections that both modify the session, or any shared resource for that matter.
The various API's and hooks provided by Pyramid are more than adequate enough to make decisions on when a session is "opened" and when a session can be considered closed (@mmerickel pointed them out in his initial reply). If you would like to use the existing hooks (for instance by creating a tween) that explicitly calls open/close on your particular session implementation you are more than welcome to do so, and it is encouraged for you to release your implementation so that others may decide to use it.
However, we as core developers and users of the Pyramid framework have not found the current session implementation lacking for the use cases where we have required it. I would argue that you are using the session storage for the wrong use cases if you need it to arbitrate which request is allowed to modify it.
Sessions storage is not meant to be bulletproof, and with two concurrent requests there is no good way to specify which request should win (is it the one that started first, or the one that completed first?).
Adding an open/close API is not something we are interested in doing, and doesn't provide anything of value, and simply complicates the existing API for no good reason.
It is not the framework's place to make a decision on how to handle two concurrent connections that both modify the session, or any shared resource for that matter.
The job of the framework is to provide interface that helps developers. How is handling state explicitly is not helpful ? Most people don't want to lose data.
Sessions storage is not meant to be bulletproof, and with two concurrent requests there is no good way to specify which request should win (is it the one that started first, or the one that completed first?).
That's why people use locking
And, why is there authentication policy that stores data in the session ? For me auth data must be bullet proof. It looks like you are just trying to be defensive and to deny mistakes.
Adding an open/close API is not something we are interested in doing,
This I can accept because i'm not a committer.
and doesn't provide anything of value, and simply complicates the existing API for no good reason.
You are ignoring the whole decades of people developing session solutions and discarding their decisions as "no value". When the whole industry is doing it, and you don't see value at all - for me it smells NIH.
Having data loss free sessions is not an opinion. Thats what most developers coming to Pyramid from other frameworks would expect - because other implementations have locking as part of the state.
You are free to disagree, but I just lost interest in contributing to Pyramid because any improvement could be discarded as "no value" without even careful consideration.