Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Add Lockable trait#18471
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
[Console] Add Lockable trait#18471
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f9d7d98 to3f9fc8aCompare| */ | ||
| publicfunctionsetAutoLock($autoLock) | ||
| { | ||
| $this->autoLock = (boolean)$autoLock; |
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 the standard is to use(bool), though usually the values are not cast like that afaik
javiereguiluz commentedApr 7, 2016
About the proposed method name: My concern: the "auto-lock" feature is an "implementation detail". What you really want is to prevent concurrent command executions. You don't care about the lock at all. If you agree, the hard part will be to find a good name for this. |
geoffrey-brier commentedApr 7, 2016
@javiereguiluz I do agree with you, I wasn't really satisfied with the naming ... what about "safe lock", "thread safe" ... ? Or setParallelizable ? |
linaori commentedApr 7, 2016
The only problem with this locking strategy is that it only works if your command is limited to 1 run per application instance. If I would deploy 2 applications which both run the command, it could cause issues. |
geoffrey-brier commentedApr 7, 2016
@iltar That's true (though the problem also occurs before this PR). What would you suggest ? Introduce some kind of interface with various strategies ? If that's the case that may be a good idea to create a new "locking" component. |
javiereguiluz commentedApr 7, 2016
@iltar@geoffrey-brier I think that'd a reasonable limitation of this feature. |
linaori commentedApr 7, 2016
If it's extendable, it's really nice (I think I've suggested this in the original feature but not sure). It's already a nice to have feature as is though. |
geoffrey-brier commentedApr 7, 2016
@javiereguiluz I think it would be reasonable too (may be be nice to enhance it later though). |
fabpot commentedApr 28, 2016
ping@lyrixx |
lyrixx commentedApr 28, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
fabpot commentedJun 14, 2016
@geoffrey-brier Do you have some time to finish this PR by taking comments into account? Thanks. |
geoffrey-brier commentedJun 14, 2016
@fabpot Sure no problem but I'm not sure to understand correctly. Should I add another method ? What about the naming ? |
f5e8f4c toca06975Comparegeoffrey-brier commentedJun 14, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@fabpot@javiereguiluz@iltar I've completely reworked the PR. Now you would do something like this: class MyCommandextends Command{protectedfunctionexecute(InputInterface$input,OutputInterface$output) {// By default it would throw a RuntimeException if the command is locked$this->lock();// Your code// The lock release is still optionnal$this->release(); }} You can also pass a boolean to the class MyCommandextends Command{protectedfunctionexecute(InputInterface$input,OutputInterface$output) {// Silently returnif ($this->lock(true)) {return; } }} |
| * | ||
| * @throws RuntimeException | ||
| */ | ||
| protected function lock($name, $blocking = false) |
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.
Do we really need the name here? As the trait is only able to deal with only one lock anyway, I'd suggest to remove the name and generate one automatically.
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 have a name depending on some arguments. But a default value (the command name) is indeed better.
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.
Fait enough, let's add a default value then.
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 about making thename able to accept full paths as well? We usually put our locks into%projectdir%/var/run so doing something like
$lockPath =null;$lockName =$name ?:$this->getName();if ($name &&$fs->isAbsolutePath($name)) {$lockPath =dirname($name);$lockName =basename($name);}
would be great. Or is that look too much?
b545b81 to8b63ed3Comparegeoffrey-brier commentedJun 23, 2016
Fixed everything |
fabpot commentedJun 23, 2016
👍 |
| }, | ||
| "require-dev": { | ||
| "symfony/event-dispatcher":"~2.8|~3.0", | ||
| "symfony/filesystem":"", |
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 version.
lyrixx commentedJun 23, 2016
@geoffrey-brier Could you update the PR description to have something that match the new implementation? Thanks. |
| */ | ||
| protected function lock($name = null, $blocking = false) | ||
| { | ||
| if (!class_exists('Symfony\Component\Filesystem\LockHandler')) { |
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.
LockHandler::class
8b63ed3 todd523cbComparegeoffrey-brier commentedJun 23, 2016
@lyrixx Done & tests are green :) |
fabpot commentedJun 23, 2016
@geoffrey-brier Looks good. Can you add a new item in the component CHANGELOG to mention this new feature for 3.2? |
dd523cb tob57a83fComparegeoffrey-brier commentedJun 23, 2016
@fabpot Done |
fabpot commentedJun 23, 2016
Thank you@geoffrey-brier. |
This PR was merged into the 3.2-dev branch.Discussion----------[Console] Add Lockable trait| Q | A| ------------- | ---| Branch? | "master"| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | none for the moment :)Hi there,Since the 2.6 the `LockHandler` class was added to ease concurrency problems. There was a nice post about [using it in your commands](http://symfony.com/blog/new-in-symfony-2-6-lockhandler).From my humble experience, I find it a bit unpleasant/time consuming to always copy/paste the same code. So here is my proposal:Before:```phpclass UpdateContentsCommand extends Command{ protected function configure() { // ... } protected function execute(InputInterface $input, OutputInterface $output) { // create the lock $lock = new LockHandler('update:contents'); if (!$lock->lock()) { $output->writeln('The command is already running in another process.'); return 0; } // Your code $lock->release(); }}```After:```phpclass MyCommand extends Command{ use LockableTrait; protected function execute(InputInterface $input, OutputInterface $output) { if (!$this->lock()) { // Here you can handle locking errors } // Your code // The lock release is still optionnal $this->release(); }}```In addition, you can optionally pass two arguments:- a string argument to change the lock name- a boolean argument to indicate if you want to wait until the requested lock is releasedCommits-------b57a83f [Console] Add Lockable trait
| * | ||
| * @return bool | ||
| */ | ||
| protected function lock($name = null, $blocking = false) |
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.
Why not declaring everythingprivate by default?
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.
👍
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.
protected looks fine to me, moving to private won't provide anything more
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.
Well, every class using this trait will now expose the used methods as an extension point which might not be what the user intended.
| /** | ||
| * Basic lock feature for commands. | ||
| * | ||
| * @author Fabien Potencier <fabien@symfony.com> |
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.
@geoffrey-brier You should have put your name here. :)
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.
@xabbuh I'm not that important ;)
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.
Of course, you're the author of this trait. :)
…abbuh)This PR was merged into the 3.2-dev branch.Discussion----------[Console] LockableTrait: change visibility to private| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#18471 (comment)| License | MIT| Doc PR |Without this change we force users to expose unnecessary extension points they may not want to provide.Commits-------eaa3bb0 LockableTrait: change visibility to private
This PR was merged into the 3.2-dev branch.Discussion----------[Console] update the author of the LockableTrait| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#18471 (comment)| License | MIT| Doc PR |Commits-------b15fec7 update the author of the LockableTrait
Uh oh!
There was an error while loading.Please reload this page.
Hi there,
Since the 2.6 the
LockHandlerclass was added to ease concurrency problems. There was a nice post aboutusing it in your commands.From my humble experience, I find it a bit unpleasant/time consuming to always copy/paste the same code. So here is my proposal:
Before:
After:
In addition, you can optionally pass two arguments: