-
Notifications
You must be signed in to change notification settings - Fork 11.4k
[5.2] SessionDatabaseHandler confirm existence before committing #12059
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
Conversation
Please squash to one commit. ;) |
Can we get a description on how this solves the issue? |
f0d84c1
to
1ea7045
Compare
@GrahamCampbell squashed @taylorotwell This indirectly solves the issue by forcing a read check before database write. Yes, there may be a chance we do an additional select query, however the 2nd query will ensure the session does not already exist in the database. The reason why this is necessary is because of database replication. I believe one of the replicas may not be completely up to date thus when reading at the beginning of the script $this->exist still equals false and then trying to write with a database insert at the end it throws the session_unique_id integrity error. Another scenario could be ajax requests. Few requests get sent out and hit the database roughly at the same time. 1st one does the insert but the 2nd request has already done the database read, but does not exist at that time. Its hard to say where the issue stems from because I only see it in production. I've seen it more often the more clients I add to the database so I definitely think its due to load and lots of concurrent requests. I agree this isn't the complete solution, however it does ensure one last check before it tries to insert when it should update. |
Maybe better solution would be to let the database figure it out with insert on duplicate or upsert depending on database type. https://dev.mysql.com/doc/refman/5.6/en/insert-on-duplicate.html |
[5.2] SessionDatabaseHandler confirm existence before committing
@deanmraz so did this fix solve your issues in production? |
@taylorotwell going good so far, I will report back Monday. |
@taylorotwell so... its not the silver bullet, but its definitely happening a lot less. We are down to a few times a week versus a few times a day. I believe the issue stems from concurrent requests via ajax calls. These requests both read that the session id does not exist, and when the requests try to insert one would fail. This solution makes it happen a lot less because it narrows the window and does another existence check right before it tries to insert. I am going to attempt using MySQL's INSERT ON DUPLICATE and will let you know how it goes. |
I just lunched site with about 5 000 000 page views per month. My fast fix was something like that (using 5.1).
Works like a charm. Need to be rewritten to fit in Laravel |
@ybr-nx - where did you put that code? Is it overwriting the base model class or did you do it elsewhere? I'm running into this issue but also running into an issue where I have a multi-key insert that I need to do the same thing to ( $model->insert[...lots of rows...]) ) and am trying to come up with the best place to override it. I'm thinking just replacing the insert() call (or making a 'replace()' call or something like that) in a trait that I can apply to the model, but am curious where you put the code to override this. |
@jdavidbakr - I edited DatabaseSessionHandler.php code under vendor as I needed some really fast fix. Still haven't figured out how/where to rewrite it.
|
@ybr-nx - I think I found a work-around that works and doesn't break with a composer update:
Then, I created a session handler class that extends the default database session handler:
In my case I'm ignoring the duplicate key error but alternatively you could re-save the session if the code is 23000. I pushed this live last Friday and I haven't had the duplicate key error since then. |
Is there any chance to update 4.2 version for this bug? |
To resolve this issue: