Mutex::release returns true when file doesn't exist
A bug in 2.1.2, submitted by eKoeS on 10 December 2010
Announcement
Symphony's issue tracker has been moved to Github.
Issues are displayed here for reference only and cannot be created or edited.
Browse
Closed#435: Mutex::release returns true when file doesn't exist
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?
My understanding is that exceptions will be thrown anyway if a lock cannot be opened due to the nature of Symphony treating E_WARNING as an exception.
This issue is closed.
https://github.com/symphonycms/symphony-2/blob/master/symphony/lib/toolkit/class.mutex.php#L24
Shouldn’t it return
false
?