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] use 'r+' for fopen (fixes issue on Solaris) #27668

Merged
merged 1 commit into from Jun 21, 2018

Conversation

Projects
None yet
3 participants
@fritzmg
Contributor

fritzmg commented Jun 21, 2018

Q A
Branch? 3.4 (also applicable to LockHandler in 2.8 and 3.3)
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes [1]
Fixed tickets -
License MIT
Doc PR -

We discovered a curious case on a specific hosting environment: the FlockStorage (or LockHandler in previous Symfony versions) could never get a file lock on existing files. So if you run a script for the first time and the lock file did not exist yet, the FlockStorage could get a lock on that file just fine. However on the second and subsequent runs, FlockStorage could never get a file lock anymore. You can follow the discussion here (if you speak German).

We have been using this little script to confirm the issue on the hosting environment:

$fileName = __DIR__ . '/file.lock';

if (!$handle = @fopen($fileName, 'r')) {
    $handle = fopen($fileName, 'x');
}

if (!$handle) {
    echo "Could not open $fileName\n";
    exit;
}

if (flock($handle, LOCK_EX | LOCK_NB)) {
    echo "Got a lock on $fileName\n";
    flock($handle, LOCK_UN | LOCK_NB);
} else {
    echo "Could not get a lock on $fileName\n";
}

Whenever file.lock already existed prior to running the script, a lock could not be made.

After contacting the hosting provider's support on this they confirmed the issue and told us they are using Solaris instead of a Linux environment. And this is supposedly why it does not work. Instead you have to use 'r+' instead of 'r' for fopen.

I was able to confirm that changing from 'r' to 'r+' fixes the issue. However I am wondering who's actually at fault here. Is it Solaris? PHP? The compiled PHP version under Solaris? The hosting provider's operating system configuration?

System information

uname -a
SunOS vlek 5.11 11.3 i86pc i386 i86pc Solaris
phpinfo:
SunOS localhost 5.10 Generic_150401-49 i86pc 
php -v
PHP 7.2.5 (cli) (built: May  4 2018 12:57:43) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies

Footnotes

[1] Previously one failed test on the first run and two failing tests on the second run with the proposed changes:

1) Symfony\Component\Lock\Tests\Store\FlockStoreTest::testSaveSanitizeName
Symfony\Component\Lock\Exception\LockStorageException: fopen(C:\Users\[…]\AppData\Local\Temp/sf.-php-echo-hello-word-.Sz2dDSf.lock)
: failed to open stream: Permission denied
2) Symfony\Component\Lock\Tests\Store\FlockStoreTest::testSaveWithDifferentKeysOnSameResources
Symfony\Component\Lock\Exception\LockStorageException: fopen(C:\Users\[…]\AppData\Local\Temp/sf.Symfony-Component-Lock-Tests-Store-
AbstractStoreTest-testSaveWithDifferentKeysOnSameResources5b2b5f00872538.64807920.2u9bH+a.lock): failed to open stream: Permission denied

The latter failed both on the first run and on the second run.

After the proposed changes from @nicolas-grekas everything works fine 👍

@fritzmg fritzmg changed the title from [RFC] use 'r+' for fopen (fixes issue on Solaris) to [RFC] use 'r+' for fopen in FlockStore (fixes issue on Solaris) Jun 21, 2018

@fritzmg fritzmg changed the title from [RFC] use 'r+' for fopen in FlockStore (fixes issue on Solaris) to [RFC][Lock] use 'r+' for fopen (fixes issue on Solaris) Jun 21, 2018

@nicolas-grekas

Legit to me: looks like Solaris doesn't allow exclusive locks on read-only file descriptors.

@@ -79,12 +79,12 @@ private function lock(Key $key, $blocking)
// Silence error reporting
set_error_handler(function ($type, $msg) use (&$error) { $error = $msg; });
if (!$handle = fopen($fileName, 'r')) {
if (!$handle = fopen($fileName, 'r+')) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 21, 2018

Member

what about doing this, to keep working if a write permission issue is raised?
if (!$handle = fopen($fileName, 'r+') ?: fopen($fileName, 'r')) {
(same below of course)

This comment has been minimized.

@fritzmg

fritzmg Jun 21, 2018

Contributor

👍 that works both on my local Windows environment and on the SunOS environment :). I'll change the PR accordingly.

@nicolas-grekas nicolas-grekas changed the title from [RFC][Lock] use 'r+' for fopen (fixes issue on Solaris) to [Lock] use 'r+' for fopen (fixes issue on Solaris) Jun 21, 2018

@fritzmg

This comment has been minimized.

Contributor

fritzmg commented Jun 21, 2018

Should I create another PR for the LockHandler in 2.8 (which can be merged to 3.3)?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 21, 2018

Yes please for 2.8 (3.3 is not maintained anymore so it won't get the fix)

@nicolas-grekas nicolas-grekas added Lock and removed RFC labels Jun 21, 2018

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

bug #27669 [Filesystem] fix file lock on SunOS (fritzmg)
This PR was merged into the 2.8 branch.

Discussion
----------

[Filesystem] fix file lock on SunOS

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

See #27668

Commits
-------

7adb641 fix file lock on SunOS
@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 21, 2018

Thank you @fritzmg.

@nicolas-grekas nicolas-grekas merged commit 9c9ae7d into symfony:3.4 Jun 21, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

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

bug #27668 [Lock] use 'r+' for fopen (fixes issue on Solaris) (fritzmg)
This PR was squashed before being merged into the 3.4 branch (closes #27668).

Discussion
----------

[Lock] use 'r+' for fopen (fixes issue on Solaris)

| Q             | A
| ------------- | ---
| Branch?       | 3.4 (also applicable to _LockHandler_ in 2.8 and 3.3)
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes [1]
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

We discovered a curious case on a specific hosting environment: the `FlockStorage` (or `LockHandler` in previous Symfony versions) could never get a file lock on **existing files**. So if you run a script for the first time and the lock file did not exist yet, the `FlockStorage` could get a lock on that file just fine. However on the second and subsequent runs, `FlockStorage` could _never_ get a file lock anymore. You can follow the discussion [here](contao/core-bundle#1551) (if you speak German).

We have been using this little script to confirm the issue on the hosting environment:
```php
$fileName = __DIR__ . '/file.lock';

if (!$handle = @fopen($fileName, 'r')) {
    $handle = fopen($fileName, 'x');
}

if (!$handle) {
    echo "Could not open $fileName\n";
    exit;
}

if (flock($handle, LOCK_EX | LOCK_NB)) {
    echo "Got a lock on $fileName\n";
    flock($handle, LOCK_UN | LOCK_NB);
} else {
    echo "Could not get a lock on $fileName\n";
}
```
Whenever `file.lock` already existed prior to running the script, a lock could not be made.

After contacting the hosting provider's support on this they confirmed the issue and told us they are using **Solaris** instead of a Linux environment. And this is supposedly why it does not work. Instead you have to use `'r+'` instead of `'r'` for `fopen`.

I was able to confirm that changing from `'r'` to `'r+'` fixes the issue. However I am wondering who's actually at fault here. Is it Solaris? PHP? The compiled PHP version under Solaris? The hosting provider's operating system configuration?

### System information
```
uname -a
SunOS vlek 5.11 11.3 i86pc i386 i86pc Solaris
```
```
phpinfo:
SunOS localhost 5.10 Generic_150401-49 i86pc
```
```
php -v
PHP 7.2.5 (cli) (built: May  4 2018 12:57:43) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies
```

### Footnotes

[1] Previously one failed test on the first run and two failing tests on the second run with the proposed changes:
```
1) Symfony\Component\Lock\Tests\Store\FlockStoreTest::testSaveSanitizeName
Symfony\Component\Lock\Exception\LockStorageException: fopen(C:\Users\[…]\AppData\Local\Temp/sf.-php-echo-hello-word-.Sz2dDSf.lock)
: failed to open stream: Permission denied
```
```
2) Symfony\Component\Lock\Tests\Store\FlockStoreTest::testSaveWithDifferentKeysOnSameResources
Symfony\Component\Lock\Exception\LockStorageException: fopen(C:\Users\[…]\AppData\Local\Temp/sf.Symfony-Component-Lock-Tests-Store-
AbstractStoreTest-testSaveWithDifferentKeysOnSameResources5b2b5f00872538.64807920.2u9bH+a.lock): failed to open stream: Permission denied
```
The latter failed both on the first run and on the second run.

After the [proposed changes](#27668 (comment)) from @nicolas-grekas everything works fine 👍

Commits
-------

9c9ae7d [Lock] use 'r+' for fopen (fixes issue on Solaris)

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

bug #27670 [Cache] Fix locking on Solaris (nicolas-grekas)
This PR was merged into the 4.2-dev branch.

Discussion
----------

[Cache] Fix locking on Solaris

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

As reported on #27668, the descriptor must be writeable on Solaris to get an exclusive lock.

Commits
-------

43da583 [Cache] Fix locking on Solaris

@fritzmg fritzmg deleted the fritzmg:patch-1 branch Jun 21, 2018

This was referenced Jun 25, 2018

fritzmg added a commit to contao/check that referenced this pull request Jul 6, 2018

@fritzmg

This comment has been minimized.

Contributor

fritzmg commented Jul 9, 2018

Sorry, my PR has a bug. Since we are using r+ now

chmod($fileName, 0444);

needs to be changed to

chmod($fileName, 0666);

I am not sure why this didn't come up before in the testing environment. I'll create follow up PRs.

nicolas-grekas added a commit that referenced this pull request Jul 9, 2018

bug #27903 [Lock] fix lock file permissions (fritzmg)
This PR was squashed before being merged into the 3.4 branch (closes #27903).

Discussion
----------

[Lock] fix lock file permissions

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

See [this comment](#27668 (comment)). Since we are using `r+` now to fix an issue on Solaris, we also need to change the file permissions when the lock file is created for the first time. Otherwise
```php
fopen($fileName, 'r+')
```
will fail due to the file permissions and while
```php
fopen($fileName, 'r')
```
will work, the subsequent locking will again fail on Solaris.

Changing the file permissions to `0666` fixes this issue. __However__ any lock file that was generated _prior_ to this change will still cause issues and would need to be manually deleted. Usually the default `sys_get_temp_dir()` location is used for the lock files and _usually_ these files are purged periodically, so it probably won't matter that much. But it still might cause some confusion since it will not be transparent, why the file lock failed on Solaris systems.

Commits
-------

23481a1 [Lock] fix lock file permissions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment