Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:masterfromlyrixx:command-lock
Sep 22, 2014
Merged

Conversation

@lyrixx
Copy link
Member

QA
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#9357 ,#3586
LicenseMIT
Doc PRhttps://github.com/symfony/symfony-docs/pull/3956/files

Code sample:

/**     * {@inheritdoc}     */protectedfunctionexecute(InputInterface$input,OutputInterface$output)    {$lockHelper =newLockHandler('/tmp/acme/hello.lock');if (!$lockHelper->lock()) {$output->writeln('The command is already running in another process.');return0;        }$output->writeln(sprintf('Hello <comment>%s</comment>!',$input->getArgument('who')));for (;;) {        }$output->writeln(sprintf('bye <comment>%s</comment>!',$input->getArgument('who')));$lockHelper->unlock();    }

process-lock

Copy link
Contributor

Choose a reason for hiding this comment

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

oh!

@lyrixx
Copy link
MemberAuthor

Actually,register_shutdown_function is resistant to fatal error. So the PID check is a bonus for linux user.

But signals don't not trigger it. Same thing for__destruct.

@cordoval
Copy link
Contributor

please add a checkbox for writing the documentation PR

Copy link
Contributor

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
Copy link
Contributor

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
Copy link
MemberAuthor

@cordoval The use case is already described in the 2 linked issues.

@malarzm
Copy link
Contributor

I think that all drivers will check for pid - isn't it better to have class AbstractLocker which could look like

abstract class AbstractLocker{    public function isLocked($name)    {        $pid=$this->retrievePid($name);        // system based check goes here    }    abstract function lock($name) {  }    abstract function unlock($name) {  }    abstract function retrievePid($name) {  }}

It should make all drivers more DRY :)

@lyrixx
Copy link
MemberAuthor

@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 useflock

@malarzm
Copy link
Contributor

I thought retrievePid would be less confusing than getPid ;) I meant use of$pid = file_get_contents($lockFile); for FilesystemLocker in retrievePid function

@stof
Copy link
Member

@malarzm what@lyrixx meant is that a locking mechanism which supports locking accross several servers (when you have 2 workers for instance) would not be based on checking a PID.

@malarzm
Copy link
Contributor

Ah right, sorry, I think I need more sleep

@cordoval
Copy link
Contributor

#3586 talks about a doctrine driver so i guess that is a request.

@lyrixx
Copy link
MemberAuthor

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)

Copy link
Contributor

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 ?

Copy link
MemberAuthor

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
Copy link
Contributor

should we add a soft suggest on the process component?

Copy link
Contributor

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
Copy link
Contributor

@lyrixx keep the interface maybe, to let others provide their own implementations (not in the core)?

Copy link
Contributor

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)

Copy link
Contributor

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

@lyrixxlyrixx changed the title[WIP][Command] Added an helper to lock a command[Command] Added an helper to lock a commandMay 28, 2014
@lyrixx
Copy link
MemberAuthor

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

Copy link
Contributor

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?

Copy link
MemberAuthor

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.

Copy link
Contributor

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?

Copy link
MemberAuthor

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
Copy link
Member

I've some concerns about this implementation:

  • it doesn't fit my use case. I need blocking locks to allow a command waiting the end of a previous one before running. Here, the non blocking mode is hardcoded (and according to the PHP doc, this mode is not supported on Windows)
  • I think it's not the responsibility of this class to kill processes. Let the developer - or the OS - doing that. And what if the killed process is a daemon or something like that? Locks aren't necessary pid files.
  • The interface was a good idea IMO (and I'll probably need "network" locks soon). A memcache lock should not be too hard to implement (even outside of core as already said).

What about

interface Lock{   publicfunctionlock($blocking =true);   publicfunctionunlock();   publicfunctionisLocked();}

@lyrixx
Copy link
MemberAuthor

it doesn't fit my use case. I need blocking locks to allow a command waiting the end of a previous one before running. Here, the non blocking mode is hardcoded (and according to the PHP doc, this mode is not supported on Windows)

Why not ;)

I think it's not the responsibility of this class to kill processes. Let the developer - or the OS - doing that. And what if the killed process is a daemon or something like that? Locks aren't necessary pid files.

We kill nothing.

The interface was a good idea IMO (and I'll probably need "network" locks soon). A memcache lock should not be too hard to implement (even outside of core as already said).

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 theLockHelper class

Then, If you need a custom lockHelper, just create a new one. There is no need to use an interface for that ;)

Copy link
Member

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.

@lyrixxlyrixxforce-pushed thecommand-lock branch 2 times, most recently from0201f9f toa7aabd2CompareSeptember 11, 2014 21:17
@lyrixx
Copy link
MemberAuthor

@Tobion Thanks for the review. I hope it's ok now ;)

Copy link
Contributor

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
Copy link
MemberAuthor

Travis and fabbot are of. shippable no and I don't know why. ping@fabpot.

@fabpot
Copy link
Member

forget about shippable, it was just a test

@lyrixx
Copy link
MemberAuthor

So it's ready for merge

@fabpot
Copy link
Member

👍

@rybakit
Copy link
Contributor

Just curious what you folks think about getting rid of the file locks and using port bindings, like it's describedhere?

@fabpot
Copy link
Member

Thank you@lyrixx.

@fabpotfabpot merged commit9ad8957 intosymfony:masterSep 22, 2014
fabpot added a commit that referenced this pull requestSep 22, 2014
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();    }```![process-lock](https://f.cloud.github.com/assets/408368/2443205/4f0bf3e8-ae30-11e3-9bd4-78e09e2973ad.png)Commits-------9ad8957 [Filesystem] Added a lock handler
@lyrixx
Copy link
MemberAuthor

@rybakit your idea is interesting, but I'm not fan of it because

  • the port could be already used by another application on production server (and not on your local machine)
  • I don't know if it work well on windows
  • It takes a port.
  • It's a bit a hack, port did not designed for that, where flock are.

@iainp999
Copy link

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
Copy link
Member

@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
Copy link

@stof oh ok, cool

weaverryan added a commit to symfony/symfony-docs that referenced this pull requestOct 18, 2014
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
@GDmacGDmac mentioned this pull requestNov 10, 2014
@lyrixxlyrixx deleted the command-lock branchDecember 8, 2015 15:21
@temple
Copy link

Could you please@lyrixx provide an interface for theLockHandler class?
It would be useful to enhance this class by using wrappers.. but this means having an interface to implement.

@lyrixx
Copy link
MemberAuthor

@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
Copy link

@lyrixx
Copy link
MemberAuthor

yes.

@temple
Copy link

Thank you very much!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

20 participants

@lyrixx@cordoval@malarzm@stof@jakzal@dunglas@nicolas-grekas@romainneutron@Tobion@jderusse@staabm@Nicofuma@fabpot@rybakit@iainp999@temple@henrikbjorn@kingcrunch@Taluu@xabbuh

[8]ページ先頭

©2009-2025 Movatter.jp