Symphony.

https://github.com/symphonycms/symphony-2/blob/master/symphony/lib/toolkit/class.mutex.php#L24

Shouldn’t it return false?

Well if a file doesn’t exist, that means there’s no lock to release, which is essentially the same as removing the lock file and returning true for the removal right?

If it returns false, that would mean that something went wrong when trying to remove a lock, so the lock would still be active, but if no lock exists, well, no lock exists?

It looks like a contradiction to me. If there’s no lock to release, then something actually went wrong because that means that such a lock is missing. Similarly, if you try to remove a file that doesn’t exists, PHP throws an error.

Is there any example where returning true is useful? I used this class in my last extension and it took me some time to understand that release always returns true even when something unexpected happens (i.e. lock not found).

This is how I think of it:

• Make a lock (Mutex::acquire)
• Do some processing
• Check to see if the lock is still exists (Mutex::acquire)
• Do some more processing
• Release the lock (Mutex::release)
• Do processing based off lock

So in this case, if Mutex::release returns false if a lock doesn’t exist, my processing would never complete because it thinks there is a lock file in place.

This is correct for if you can’t unlink a file (ie. unlink returns false), the lock file is still there, so no further processing should occur. In your application I suppose this would be an Exception or Error, because something has gone wrong and not removed the lock.

By returning true if a lock was removed, or if lock never existed is exactly the same to me. I’m not sure why you would separate the logic between true for ‘lock removed’ or true for ‘lock never existed so I need to do something’.

By changing it to false it would mean ‘something went wrong removing the lock, ie. lock exists)’ or ‘no lock existed in the first place, ie. lock doesn’t exist’, which I think it much harder to do logic on because it doesn’t really tell you anything.

To check if a lock exists, you really should be using Mutex::acquire as this will also check if the lock is expired.

Thank you for taking the time to reply, Brendan.

I’m not sure why you would separate the logic between true for ‘lock removed’ or true for ‘lock never existed so I need to do something’.

By changing it to false it would mean ‘something went wrong removing the lock, ie. lock exists)’ or ‘no lock existed in the first place, ie. lock doesn’t exist’, which I think it much harder to do logic on because it doesn’t really tell you anything.

This is a good point, but I think that the same holds true for Mutex::acquire. If something goes wrong while creating the lock file (i.e. fopen returns false) then false is returned to the caller. If the lock already exists but it hasn’t expired yet, false is returned as well. In this context how can I know the reason why I’ve got false? That’s why I used Mutex::release for checking the existence of a lock file in the first place.

Maybe we could expose the Mutex::__lockExists function? We can use an additional param of something like raw to trigger Mutex::__generateLockFileName inside the Mutex::__lockExists function. Would that help remove ambiguity?

I like the idea, Brendan. An alternative would be to create a public lockExists method that calls __generateLockFileName and __lockExists, so that there’s no need for a $raw parameter.

What about throwing an exception when a lock file can’t be created/written/opened due to permissions?