Project

General

Profile

Actions

Bug #24342

closed

t3lib_lock still sometimes has a race condition on deleting locks.

Added by Björn Pedersen over 13 years ago. Updated about 9 years ago.

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

0%

Estimated time:
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)


Files

t3lib_class.t3lib_lock.php.patch (12.1 KB) t3lib_class.t3lib_lock.php.patch Administrator Admin, 2010-12-15 13:15
16749_v2.patch (21.5 KB) 16749_v2.patch Administrator Admin, 2010-12-16 14:18
16749-v3.diff (30.4 KB) 16749-v3.diff Administrator Admin, 2010-12-20 10:34
16749-v3a.diff (30.4 KB) 16749-v3a.diff Administrator Admin, 2010-12-20 10:58

Related issues 2 (0 open2 closed)

Related to TYPO3 Core - Feature #18557: Integrate locking by database in t3lib_lockRejectedBenni Mack2008-04-03

Actions
Has duplicate TYPO3 Core - Bug #32282: typo3temp/locks warning. unlink function returns warningClosed2011-12-03

Actions
Actions #1

Updated by Björn Pedersen over 13 years ago

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

Actions #2

Updated by Björn Pedersen over 13 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.

Actions #3

Updated by Ernesto Baschny over 13 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.

Actions #4

Updated by Björn Pedersen over 13 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.

Actions #5

Updated by Ernesto Baschny over 13 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
Actions #6

Updated by Björn Pedersen over 13 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.

Actions #7

Updated by Ernesto Baschny over 13 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?

Actions #8

Updated by Björn Pedersen over 13 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.

Actions #9

Updated by Björn Pedersen over 13 years ago

3a fixes a minor typo

Actions #10

Updated by Björn Pedersen about 13 years ago

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

Actions #11

Updated by Xavier Perseguers over 12 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?

Actions #12

Updated by Xavier Perseguers over 12 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
Actions #13

Updated by Helmut Hummel over 12 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

Actions #14

Updated by Björn Pedersen over 12 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.

Actions #15

Updated by Helmut Hummel over 12 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.

Actions #16

Updated by Oliver Hader over 12 years ago

  • Target version changed from 4.7.0-alpha2 to 4.7.0-alpha3
Actions #17

Updated by Steffen Ritter about 12 years ago

  • Target version changed from 4.7.0-alpha3 to 4.7.0-beta1
Actions #18

Updated by Steffen Ritter about 12 years ago

  • Target version changed from 4.7.0-beta1 to 4.7.0-beta2
Actions #19

Updated by Björn Pedersen about 12 years ago

@Helmut:
Any news on this one?

Actions #20

Updated by Helmut Hummel about 9 years ago

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

Updated by Markus Klein about 9 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

Actions

Also available in: Atom PDF