8000 [4.2] Fix file locking issue with Laravel file sessions by avi123 · Pull Request #6848 · laravel/framework · GitHub
[go: up one dir, main page]
Skip to content

[4.2] Fix file locking issue with Laravel file sessions #6848

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[4.2] Fix file locking issue with Laravel file sessions #6848

wants to merge 3 commits into from

Conversation

avi123
Copy link
@avi123 avi123 commented Dec 29, 2014

No description provided.

@avi123 avi123 mentioned this pull request Dec 29, 2014
Closed
@GrahamCampbell
Copy link
Member

There are quite a few cs issues here. I'm going to comment on them all...

GrahamCampbell
GrahamCampbell reviewed Dec 29, 2014
View reviewed changes
src/Illuminate/Session/FileSessionHandler.php
@@ -3,7 +3,15 @@
use Symfony\Component\Finder\Finder;
use Illuminate\Filesystem\Filesystem;

class FileSessionHandler implements \SessionHandlerInterface {
class FileSessionHandler implements \SessionHandlerInterface
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert

@GrahamCampbell GrahamCampbell changed the title Fix file locking issue with Laravel file sessions [4.2] Fix file locking issue with Laravel file sessions Dec 29, 2014
@taylorotwell
Copy link
Member

I can't recreate the bug people a few people have mentioned. I cloned down the one guy's test repository and ran it probably 30-40 times and never saw the bug once. What am I missing?

@avi123
Copy link
Author
avi123 commented Dec 30, 2014

I don't know anything about the test repo that you're mentioning, but we encountered the bug when having a browser, representing a logged in user, make 10 - 15 concurrent requests. Not always, but sometimes 1 of the concurrent requests would fail claiming to not be logged in - the session ID would be different from all of the other requests.

@GrahamCampbell
Copy link
Member

@avi123 Could you fix those issues that I've highlighted please.

@GrahamCampbell
Copy link
Member

Is anyone able to test this please and confirm that this does work? If we can't get this 100% clear, then this will have to be retargeted to 5.0. Once the psr-2 changes go into laravel 5.0, laravel 4.2 will become locked for all improvements or edge cases fixes like this.

@GrahamCampbell
Copy link
Member

Also note, for anyone coming to this and reading my last comment here, Laravel 5 never moved to PSR-2, and there are no plans to in the near future.

@loren138
Copy link

I'm assuming this never got worked into anything. I've just run into this issue and am in need of getting it fixed. I've put together a sample repository here: https://github.com/wiseloren/laravel-race That shows the problem. I've only tested with the file driver since I didn't have a database handy for the test project. I'm willing to work on the code, though, I'm guessing I'll need to find and fix the correct files in 5.0 instead of 4.2.

@loren138 loren138 mentioned this pull request Feb 21, 2015
Closed
@loren138
Copy link

I can confirm that this fix fixes the issues in the laravel race repository causing everything to properly be incremented.

loren138 referenced this pull request Feb 23, 2015
@taylorotwell
Use exclusive lock on session write.
d6648e2
@cedricve
Copy link
cedricve commented Feb 7, 2017

Nice, fixes my issue!

@cedricve
Copy link
cedricve commented Feb 7, 2017 •
edited
Loading

@loren138 I know this has been a while, but I've created a ServiceProvider:

https://github.com/cedricve/lockfile

@cedricve cedricve mentioned this pull request Feb 7, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@avi123 @GrahamCampbell @taylorotwell @loren138 @cedricve
0