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

[Lock] Remove released semaphore #27357

Merged
merged 1 commit into from Jun 8, 2018

Conversation

Projects
None yet
5 participants
@jderusse
Contributor

jderusse commented May 23, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #27356
License MIT
Doc PR NA

This PR remove the semaphore with sem_remove. By removing without releasing the semaphore, all pending blocking acquiring will fail that's why the acquire method has also been update to handle such case

@stof

This comment has been minimized.

Member

stof commented May 23, 2018

Can we have tests reproducing the issue ? AFAIK, all tests of the SemaphoreStore are green with the existing implementation, so having it non-working means that the testsuite is not covering things properly.

@stof

This comment has been minimized.

Member

stof commented May 23, 2018

and you worked on an outdated version of Symfony master (or you targeted the wrong branch when opening the PR), as this conflicts.

@jderusse jderusse changed the base branch from master to 3.4 May 23, 2018

@jderusse

This comment has been minimized.

Contributor

jderusse commented May 23, 2018

Added a test case and fix target branch

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone May 23, 2018

@stof

This comment has been minimized.

Member

stof commented May 24, 2018

I'd rather see a test about the behavior of the store (to prevent the bug beign reported) rather than a test enforcing this specific implementation (which does not prove that this implementation is the right one, but forbids replacing it)

@jderusse

This comment has been minimized.

Contributor

jderusse commented May 24, 2018

@stof unless I missed a point, the bug reported is semaphores are not removed which fill the system device and end with an error "no space left on device". It's kinda tracking a memory leak or a un-removed files which fill a disk, that's why the bug exists whereas every tests were green.

The test asserts that amount of semaphores used on the OS does not increase when the lock is acquired then released, regardless of the implementation.
Moreover a second assertion validate the test itself by checking that the amount of semaphore has been increased when the lock was acquired, which proves that this is the right way to count the amount of semaphore used by the OS

An other way to test it would be to reproduce the reported bug. Meaning either

  • creating more than 32 000 semaphores (default value) which take ~ 2 minutes on my machine
  • reducing this limit by changing a kernel parameter in /etc/sysctl.conf (requires sudo and restart)

Do you see another way to test the behavior?

@stof

This comment has been minimized.

Member

stof commented May 24, 2018

Is sem_release a useless API then ?

@jderusse

This comment has been minimized.

Contributor

jderusse commented May 24, 2018

Wouldn't say so.

sem_release is the right way to release a lock, next pending sem_acquire will softly take the lock. BUT, it does not free the resource. If nobody want to acquire the lock, the resource will stay here (consuming 1 slot on the 32 000 available). AND because of cross concurrency, you can not be sure that nobody else use the semaphore, then you can not remove the resource safely...

sem_remove is the brute way to the release the resource. By removing it: every pending sem_acquire will fail.

If your application can control the amount of distinguished resource, you should sem_release. If not, the single workaround I found is to use the sem_remove and deal with the failure.

For the record, we have exactly the same issue with the original implementation of flock, the file is not removed and consume 1 inode, but with more than millions of inodes available, this bug has not yet been reported...

@javiereguiluz javiereguiluz added the Lock label May 24, 2018

while ($blocking && !$acquired) {
$resource = sem_get($keyId);
$acquired = @sem_acquire($resource, !$blocking);
}
}
if (!$acquired) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 4, 2018

Member

Just wondering: can this happen at all now? do we need some sort of timeout in the while above?

This comment has been minimized.

@jderusse

jderusse Jun 4, 2018

Contributor

yes, if \PHP_VERSION_ID >= 50601 and !$blocking

while ($blocking && !$acquired) {
$resource = sem_get($keyId);
$acquired = @sem_acquire($resource, !$blocking);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 4, 2018

Member

the value of $blocking is known here

This comment has been minimized.

@jderusse

jderusse Jun 4, 2018

Contributor

fixed

$acquired = sem_acquire($resource, !$blocking);
$acquired = @sem_acquire($resource, !$blocking);
while ($blocking && !$acquired) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 4, 2018

Member

This loop can be moved outside of the if/else

} else {
$acquired = sem_acquire($resource, !$blocking);
$acquired = @sem_acquire($resource, !$blocking);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 4, 2018

Member

By swapping the order of the if/else, we can simplify the condition: if (PHP>=...) elseif !$blovking throw

This comment has been minimized.

@jderusse

jderusse Jun 5, 2018

Contributor

done

@nicolas-grekas nicolas-grekas changed the title from Remove released semaphore to [Lock] Remove released semaphore Jun 8, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 8, 2018

Thank you @jderusse.

@nicolas-grekas nicolas-grekas merged commit 77b9f90 into symfony:3.4 Jun 8, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jun 8, 2018

bug #27357 [Lock] Remove released semaphore (jderusse)
This PR was merged into the 3.4 branch.

Discussion
----------

[Lock] Remove released semaphore

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #27356
| License       | MIT
| Doc PR        | NA

This PR remove the semaphore with `sem_remove`. By removing without releasing the semaphore, all pending blocking acquiring will fail that's why the acquire method has also been update to handle such case

Commits
-------

77b9f90 Remove released semaphore

This was referenced Jun 25, 2018

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