-
-
Notifications
You must be signed in to change notification settings - Fork 131
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
issue#348. Fixed CSRF tokens not sessioned when using scope and memory_sessions #349
base: master
Are you sure you want to change the base?
issue#348. Fixed CSRF tokens not sessioned when using scope and memory_sessions #349
Conversation
…tialized only once on server run, not for every handler
let sql_sessions = Sql_session.sql_sessions | ||
let memory_sessions = Session.memory_sessions () | ||
let cookie_sessions = Session.cookie_sessions () | ||
let sql_sessions = Sql_session.sql_sessions () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance, this looks like whatever is delayed by the extra unit
parameter, will be triggered once per process, rather than once per server (or once per scope
). With this change, can there be two concurrent uses of two different memory_session
middlewares in one process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your questions. Right after you answered them, I realised that my solution is wrong.
With this change, can there be two concurrent uses of two different memory_session middlewares in one process?
No, there can't be.
I suppose that, a correct fix is to change the way how we use session middlewares. I mean like this Dream.memory_sessions ()
. But in this case we would need to change examples and documentation. I haven't dare to make such global changes without discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you give some consideration to whether there is an alterative way to fix this without complicating usage in this way? And if not, then it's fine to add a ()
parameter to the session middlewares. Thank you!
@@ -1583,15 +1583,15 @@ val invalidate_session : request -> unit promise | |||
|
|||
(** {2 Back ends} *) | |||
|
|||
val memory_sessions : ?lifetime:float -> middleware | |||
val memory_sessions : middleware |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to the ?lifetime
parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was forced to eliminate it for saving existing way of usage session middlewares. In other way every usage of session middleware would look like Dream.memory_sessions ()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this parameter has to be kept, and, in light of the other comment, can be kept.
Changed the signature of sessions' functions so that they will be initialized only once on server run, not for every handler. Unfortunately, there's a side effect - session middlewares will be initialized even if they are not used in an application.