Bug #24342

t3lib_lock still sometimes has a race condition on deleting locks.

Added by Björn Pedersen almost 9 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
-
Target version:
-
Start date:
2010-12-15
Due date:
% Done:

0%

TYPO3 Version:
4.7
PHP Version:
5.3
Tags:
Complexity:
Is Regression:
No
Sprint Focus:

Description

The problem of already deleted lock files still exists. * Solution : Add a "static" lock class and implement a lockFile-Lock.

Another problem was, that locks that never were acquired were never released properly:
The early return in release returned wrongly
  • add a destruct member and use it as second flag for the early return

The patch probably still needs some cleanup, e.g. where to keep the lockLock instance.
(issue imported from #M16749)

t3lib_class.t3lib_lock.php.patch View (12.1 KB) Administrator Admin, 2010-12-15 13:15

16749_v2.patch View (21.5 KB) Administrator Admin, 2010-12-16 14:18

16749-v3.diff View (30.4 KB) Administrator Admin, 2010-12-20 10:34

16749-v3a.diff View (30.4 KB) Administrator Admin, 2010-12-20 10:58


Related issues

Related to TYPO3 Core - Feature #18557: Integrate locking by database in t3lib_lock Accepted 2008-04-03
Duplicated by TYPO3 Core - Bug #32282: typo3temp/locks warning. unlink function returns warning Closed 2011-12-03

History

#1 Updated by Björn Pedersen almost 9 years ago

v2: split into three files (one for each class).

#2 Updated by Björn Pedersen almost 9 years ago

related also to: http://bugs.typo3.org/view.php?id=8010 ,

especially as there has been a whish to split each in a separate class.

#3 Updated by Ernesto Baschny almost 9 years ago

Maybe it would be easier to get this thing splitted in two separate patches:

1) splitting up without changing the functionality

2) fixing the race condition part

I think mixing both causes some confusion in the reviewing process (see comment from Dmitry).

And how does it relate to #18557? Will you be able to integrate Olly's addition in your splitup?

But maybe I understood something wrong, as I haven't really read through the code.

#4 Updated by Björn Pedersen over 8 years ago

Just to summarize current status and my changes:

currently we have t3lib_lock with all the switches in it.

I introduced a new abstract lock class ( copying the current t3lib_lock and only removing the acquire and release methods) gets extended to get a functionally equivalent t3lib_lock ( with the old acquire and release code) again.
Additionally I introduced a second lock class t3lib_lock_static, also extending the abstract class with release and acquire method.

The static lock does not remove it's ressources, so it can be used as a global lock.
To get rid of the race condition t3lib_lock got an instance ( stored in $globals ) of the static class to protect access to it's lock ressource.

#5 Updated by Ernesto Baschny over 8 years ago

It sounds like you have:

- part 1 is a refactoring of t3lib_lock into an abstract and a (backwards compatible) concrete class
- part 2 is a bugfix in t3lib_lock
- part 3 is a new feature t3lib_lock_static

So it would be very great to have this split up in three patches that does that in that order so that the history of changes reflect that.

Maybe before you do that work, we should clarify with Olly if we could have the t3lib_lock_Database (#18557) using your refactoring too.

About the naming and file location this is more or less how our CGL tells us to do it:

  • t3lib_lock = t3lib/class.t3lib_lock.php
  • t3lib_lock_AbstractLock = t3lib/lock/class.t3lib_lock_abstractlock.php
  • t3lib_lock_Static = t3lib/lock/class.t3lib_lock_static.php
  • t3lib_lock_Database = t3lib/lock/class.t3lib_lock_database.php

#6 Updated by Björn Pedersen over 8 years ago

I can integrate patch from Olly in my current rewrite. If there is a decision to generate new sub-classes, it will take more work and reviews.

#7 Updated by Ernesto Baschny over 8 years ago

Hi Olly, you might want to take a look at this issue. What do you think of this? Could this idea of the abstract lock be re-used or made more generic? Maybe we should create a LockInterface above that all?

#8 Updated by Björn Pedersen over 8 years ago

- part 1 is a refactoring of t3lib_lock into an abstract and a (backwards compatible) concrete class
- part 2 is a new feature t3lib_lock_static (needed for the bug fix)
- part 3 is a bugfix in t3lib_lock

For the classes:
The split-up suggested by Dimitry would give
t3lib_lock : A proxy for the sub-classes
t3lib_lock_Simple
t3lib_lock_Flock,
t3lib_lock_Seamphore
t3lib_lock_Database

and static variants of all sub-classes.

#9 Updated by Björn Pedersen over 8 years ago

3a fixes a minor typo

#10 Updated by Björn Pedersen over 8 years ago

Currently postponing it to4.6. I found some problems with non-triggering autoloader.

#11 Updated by Xavier Perseguers almost 8 years ago

  • Target version changed from 0 to 4.7.0-alpha2

Unfortunately this patch was not pushed to 4.6 but as refactoring makes sense it may hopefully be part of 4.7. Could you please rework it if needed and push it to Gerrit ASAP?

#12 Updated by Xavier Perseguers almost 8 years ago

  • Status changed from New to Accepted

There is indeed a race condition, as reported in this syslog (after slightly modifying Core to add additional information):

05-12-11 15:24 - cms: Locking: Failed to acquire lock: Lock file could not be created
(/path/to/typo3temp/locks/f288221509d8f33b8e85fbfd25731733)
05-12-11 15:24 - Core: Error handler (FE): PHP Warning: unlink(/path/to/typo3temp/locks/
f288221509d8f33b8e85fbfd25731733) [<a href='function.unlink'>function.unlink</a>]: No such
file or directory in /path/to/t3lib/class.t3lib_lock.php line 21

#13 Updated by Helmut Hummel almost 8 years ago

  • Assignee set to Helmut Hummel
  • TYPO3 Version changed from 4.5 to 4.7
  • PHP Version changed from 5.2 to 5.3

I take care of this

#14 Updated by Björn Pedersen almost 8 years ago

Some more remarks for my patch:

After looking at it, there still some things not optimal:
  • The available locking methods are hardcoded, they should probably be declared in TYPO3CONF so that extensions can add locking methods.
  • The return value of acquire seems still senseless to me. The information wether we had to wait for lock or not is not really useful, a success/failure in indication would be more useful (like a typical posix mutex).

I will probably not have much time to work on teh code the next days.

#15 Updated by Helmut Hummel almost 8 years ago

Tanks for your ground work so far.

Björn Pedersen wrote:

After looking at it, there still some things not optimal:

Right. I've seen it. I'll take care of it. I already did some cleanup of your patch already but I'm not yet happy with the result. Once I'm ready, I'll push it to Gerrit.

#16 Updated by Oliver Hader over 7 years ago

  • Target version changed from 4.7.0-alpha2 to 4.7.0-alpha3

#17 Updated by Steffen Ritter over 7 years ago

  • Target version changed from 4.7.0-alpha3 to 4.7.0-beta1

#18 Updated by Steffen Ritter over 7 years ago

  • Target version changed from 4.7.0-beta1 to 4.7.0-beta2

#19 Updated by Björn Pedersen over 7 years ago

@Helmut:
Any news on this one?

#20 Updated by Helmut Hummel over 4 years ago

  • Assignee deleted (Helmut Hummel)
  • Is Regression set to No

#21 Updated by Markus Klein over 4 years ago

  • Status changed from Accepted to Closed
  • Target version deleted (4.7.0-beta2)

4.7 is not supported anymore and locking has been reworked in CMS 6.2 and will be even more reworked with CMS 7

Also available in: Atom PDF