Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Filesystem] Added a LockHandler#10475
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
oh!
lyrixx commentedMar 18, 2014
Actually, But signals don't not trigger it. Same thing for |
cordoval commentedMar 18, 2014
please add a checkbox for writing the documentation PR |
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.
missing inheritdoc doc blocks?
cordoval commentedMar 18, 2014
should we add a soft suggest on the process component? Of course redis sounds like a good fit, about the drivers would be good to first get the use case right, so could you please provide an explanation like@webmozart does ? 👶 |
lyrixx commentedMar 18, 2014
@cordoval The use case is already described in the 2 linked issues. |
malarzm commentedMar 18, 2014
I think that all drivers will check for pid - isn't it better to have class AbstractLocker which could look like It should make all drivers more DRY :) |
lyrixx commentedMar 18, 2014
@malarzm Retrieving the PID is only possible when everything is on the same machine. So it does not make sens. More over, I think I will remove this implementation and use |
malarzm commentedMar 18, 2014
I thought retrievePid would be less confusing than getPid ;) I meant use of |
stof commentedMar 18, 2014
malarzm commentedMar 18, 2014
Ah right, sorry, I think I need more sleep |
cordoval commentedMar 18, 2014
#3586 talks about a doctrine driver so i guess that is a request. |
lyrixx commentedMar 18, 2014
Actually, I will remove the interface. And so I will not implement doctrine or redis. Distributed lock is really too hard, with too many edge case. So We will keep something very simple, that work only for one server. (@romainneutron found this gist for windows) |
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.
Any error handling? What about permission issues ?
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.
already checked in the constructor.
cordoval commentedMar 18, 2014
should we add a soft suggest on the process component? |
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.
Maybe name it a bit more generic..? It currently allows to lock anything with a given name... Mentioningcommand here could make users feel it only works withConsoleCommands or things like that..
jakzal commentedMar 18, 2014
@lyrixx keep the interface maybe, to let others provide their own implementations (not in the core)? |
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.
you could use option-p :sprintf('ps -p %d', $pid)
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.
orkill -0 $pid and check the exitcode. This could be done withposix_kill is this function is available
lyrixx commentedMay 28, 2014
Hello. I have updated the code. All code came from@romainneutron ;) Now, we supportonly one kind of driver (file-system), So there is no need for an interface. It is done on purpose. Using a lock feature in a network is a nightmare. So we do not want to support it at all. ping@dunglas |
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.
Success of flock is shown by return value and therefore should be checked against?
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.
not here. See code above... Here we release the lock we get.
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.
But the release could also fail, couldn't it?
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 don't think so.
dunglas commentedMay 28, 2014
I've some concerns about this implementation:
What about interface Lock{ publicfunctionlock($blocking =true); publicfunctionunlock(); publicfunctionisLocked();} |
lyrixx commentedMay 28, 2014
Why not ;)
We kill nothing.
Actually, It's VERY VERY hard to implement. What if your command segfault ? nobody will release the lock... The only almost working implementation implies heart beat... And this is hard to implements. See the "warning" section I added to the Then, If you need a custom lockHelper, just create a new one. There is no need to use an interface for that ;) |
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.
If no interface, this statement must me removed.
0201f9f toa7aabd2Comparelyrixx commentedSep 11, 2014
@Tobion Thanks for the review. I hope it's ok now ;) |
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.
local variable $file is not necessary. just use$this->file
lyrixx commentedSep 22, 2014
Travis and fabbot are of. shippable no and I don't know why. ping@fabpot. |
fabpot commentedSep 22, 2014
forget about shippable, it was just a test |
lyrixx commentedSep 22, 2014
So it's ready for merge |
fabpot commentedSep 22, 2014
👍 |
rybakit commentedSep 22, 2014
Just curious what you folks think about getting rid of the file locks and using port bindings, like it's describedhere? |
fabpot commentedSep 22, 2014
Thank you@lyrixx. |
This PR was merged into the 2.6-dev branch.Discussion----------[Filesystem] Added a LockHandler| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#9357 ,#3586| License | MIT| Doc PR |https://github.com/symfony/symfony-docs/pull/3956/filesCode sample:```php /** * {@inheritdoc} */ protected function execute(InputInterface $input, OutputInterface $output) { $lockHelper = new LockHandler('/tmp/acme/hello.lock'); if (!$lockHelper->lock()) { $output->writeln('The command is already running in another process.'); return 0; } $output->writeln(sprintf('Hello <comment>%s</comment>!', $input->getArgument('who'))); for (;;) { } $output->writeln(sprintf('bye <comment>%s</comment>!', $input->getArgument('who'))); $lockHelper->unlock(); }```Commits-------9ad8957 [Filesystem] Added a lock handler
lyrixx commentedSep 22, 2014
@rybakit your idea is interesting, but I'm not fan of it because
|
iainp999 commentedSep 26, 2014
Hi guys. I have a similar project already (https://github.com/iainp999/lockman - approach is slightly different, happy for feedback) but would be interested in contributing to a more generic solution for SF2. |
stof commentedSep 26, 2014
@iainp999 We explicitly rejected adding distributed locking in core. People needing such feature would then need to use a library providing such feature (either your library, or one of the others available on Packagist) |
iainp999 commentedSep 26, 2014
@stof oh ok, cool |
This PR was merged into the master branch.Discussion----------[Command] Added LockHelper| Q | A| ------------- | ---| Doc fix? | no| New docs? | yessymfony/symfony#10475| Applies to | Symfony version 2.6+| Fixed tickets | -Commits-------909a294 [Filesystem] Added documentation for the new LockHandler2bf8c55 [Filesystem] Moved current document to a dedicated folder
temple commentedApr 3, 2017
Could you please@lyrixx provide an interface for the |
lyrixx commentedApr 3, 2017
@temple We do not provide an interface on purpose. It works only on one single server. If you want something more sophisticated, please use the Lock component instead. |
temple commentedApr 3, 2017
Are you referring this one,@lyrixx ? |
lyrixx commentedApr 3, 2017
yes. |
temple commentedApr 3, 2017
Thank you very much! |
Code sample: