Skip to content
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

Remove Exclusive Lock That Breaks NFS Caching #25337

Merged
merged 1 commit into from
Dec 6, 2017

Conversation

brianfreytag
Copy link
Contributor

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #25336
License MIT
Doc PR n/a

#24960 introduced an issue with NFS mounts that do not support exclusive locks. This reverts that change.

FYI @kalessil

This removes the exclusive lock that was introduced in symfony#24960.

NFS File Systems do not support exclusive locking, and generates a lot
of errors every time you try to do anything with che cache.
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Dec 5, 2017
@stof
Copy link
Member

stof commented Dec 5, 2017

Do we use LOCK_EX in other places of Symfony which may be affected too ?

@brianfreytag
Copy link
Contributor Author

@stof Good question. Let me do a search on the codebase. I'll get back with you in a moment.

@brianfreytag
Copy link
Contributor Author

It appears it's used in several other places:

Symfony\Component\Filesystem\LockHandler::lock (100)
Symfony\Component\HttpKernel\Store::lock|isLocked (75|120)
Symfony\Component\Lock\Store\FlockStore::lock (100)
Symfony\Component\Process\Tests\ProcessTest::testIncrementalOutput (399|402)

There are also a few in non-Symfony components that are tied closely to Symfony:
Doctrine\ORM\Cache\Region::lock (242)
Monolog\Handler\DeduplicationHandler::collectLogs (141)
Monolog\Handler\StreamHandler::write (113)

Honestly, I've never run into issues with these and I don't think any of these locks are new, so I don't know that they are that big of a deal. It looks to me that the vast majority of these check to make sure we can get a lock with flock() before it attempts to do anything. If not, it just returns null or false or wahtever. I didn't dig closely into each one, but they may handle things differently if they're not able to get a lock.

I'm open to opinions. Like I said, I've never had any issues with the rest of these.

@stof
Copy link
Member

stof commented Dec 5, 2017

Well, place implementing a lock indeed must use LOCK_EX. So this list looks fine to me.

@brianfreytag
Copy link
Contributor Author

@stof my thought exactly. Let me know if you need anything else from me.

@nicolas-grekas
Copy link
Member

Thank you @brianfreytag.

@nicolas-grekas nicolas-grekas merged commit a7ac100 into symfony:3.4 Dec 6, 2017
nicolas-grekas added a commit that referenced this pull request Dec 6, 2017
This PR was merged into the 3.4 branch.

Discussion
----------

Remove Exclusive Lock That Breaks NFS Caching

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

#24960 introduced an issue with NFS mounts that do not support exclusive locks. This reverts that change.

FYI @kalessil

Commits
-------

a7ac100 Remove LOCK_EX That Breaks Cache Usage on NFS
@brianfreytag brianfreytag deleted the 3.4_exclusive_locks branch December 6, 2017 12:45
@brianfreytag
Copy link
Contributor Author

Not to be pushy, but is there a timetable for a 3.4.2 release?

This was referenced Dec 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants