Midgard CMS: lock() and _on

lock() and _on_updated()

Sat July 19 2008 11:15:04 UTC

Hi!

after updating MidCOM to the latest version, I noticed some strange
behavior in my application. It turned out that this came from the fact
that locking an object triggers the _on_updated functions of the
respective DBA classes, i.e. as soon as you click on "edit" for any
object, the pre and post update routines are executed.

Personally, I find this rather unintuitive, as locking is not really an
update. So I was wondering: Is there a way to disable the execution of
the _on_updated and _on_updating functions for locking? Alternatively,
I'm thinking about doing something like

function _on_updated()
{
if ($this->is_locked())
{
return true;
}
// otherwise, do your thing
}

But I'm unsure if I could shoot myself in the foot with that (i.e. could
there be a timing problem in that the object gets saved without
unlocking it?)

Bye,

Andreas
_______________________________________________
dev mailing list
dev@lists.midgard-project.org
http://lists.midgard-project.org/mailman/listinfo/dev

Reply
Report abuse

Andreas Flack

Re: [midgard-dev] lock() and _on_updated()

Sat July 19 2008 13:50:04 UTC

P.S.: Furthermore, there is a but in the is_locked logic:

is_locked (from line 758):

// Object hasn't been marked to be edited
if ($this->get('locked') === 0)
{
// Make sure locker is empty too
if ($this->get('locker') != '')
{
$this->set('locker', '');

get (from line 161):
if (! array_key_exists($key, $this->_cache))
{
$this->_retrieve_value($key);
}

_retrieve_value (from line 393):

// Person properties
case 'creator':
case 'revisor':
case 'locker':
case 'approver':
case 'authors':
$value = $this->object->metadata->$key;
if (!$value)
{
// Fall back to "Midgard admin" if person is not found
$value = 1;
}
break;

So $this->get('locker') will never return '', but 1 instead, causing the
object to be needlessly updated each time is_locked() is called.

If you move case 'locker' up to the Time-based properties, this goes
away. but I haven't really checked if it breaks anything else.

Bye,

Andreas

Andreas Flack schrieb:
> Hi!
>
> after updating MidCOM to the latest version, I noticed some strange
> behavior in my application. It turned out that this came from the fact
> that locking an object triggers the _on_updated functions of the
> respective DBA classes, i.e. as soon as you click on "edit" for any
> object, the pre and post update routines are executed.
>
> Personally, I find this rather unintuitive, as locking is not really an
> update. So I was wondering: Is there a way to disable the execution of
> the _on_updated and _on_updating functions for locking? Alternatively,
> I'm thinking about doing something like
>
> function _on_updated()
> {
> if ($this->is_locked())
> {
> return true;
> }
> // otherwise, do your thing
> }
>
> But I'm unsure if I could shoot myself in the foot with that (i.e. could
> there be a timing problem in that the object gets saved without
> unlocking it?)
>
>
> Bye,
>
> Andreas
> _______________________________________________
> dev mailing list
> dev@lists.midgard-project.org
> http://lists.midgard-project.org/mailman/listinfo/dev

_______________________________________________
dev mailing list
dev@lists.midgard-project.org
http://lists.midgard-project.org/mailman/listinfo/dev

Reply
Report abuse

Andreas Flack

Re: [midgard-dev] lock() and _on_updated()

Sun July 20 2008 17:40:04 UTC

Hi!

Sorry for spamming, but the saga continues :-)

As mentioned in my previous post, there is a bug which causes is_locked
to update the object each time it is called, which until recently also
produced an RCS revision, thus partly explaining the multiple revisions
I saw each time I edited something.

Since locking an object (even with the abovementioned bug removed)
triggers the _on_updating and _on_updated handlers to execute, I added
this to all the relevant functions:

function _on_updated()
{
if ($this->metadata->locked)
{
return;
}

The theory was that _on_updated should only be executed when a DM2 lock
is released, i.e. after the saving is complete. Of course, this is not
perfect, since it could happen that there is a leftover lock, and the
object is updated with the normal update() call, i.e. without DM2
involved. Like I wrote in my first post, the best solution would be if
locking and unlocking wouldn't trigger the event handlers at all. The
second best thing would be if there was a global config option to
disable locking altogether.

But apart from that, I have one more problem. The code above doesn't
even perform it's basic task, because it turns out that each locking and
unlocking operation performs two updates, because locker and locked are
written separately. So one edit with DM2 updates the object five or six
times:

1: is_locked() (produces one update to locker if the object is unlocked)
2: lock() (locker is set)
3: lock() (locked is set)
4: update() (data is written)
5: unlock() (locker is unset)
6: unlock() (locked is unset)

So the above code has to look like this:

function _on_updated()
if ($this->metadata->locked || $this->metadata->locker == '')
{
return;
}

Which is of course quite awkward.

So I was wondering: Is there any real reason why locker and locked are
written separately? If the locking was atomic, db i/o would be reduced
and the bug in is_locked would disappear as well (because it only occurs
in a consistency check when the object is unlocked and the function
thinks that locker is still set). The above would then look like this:

1: lock() (locker and locked are set)
2: update() (data is written)
3: unlock() (locker and locked are unset)

Maybe this could even be further optimized by removing the lock in the
update step:

1: lock() (locker and locked are set)
2: update() (locker and locked are unset, data is written)

But reducing the six updates to three would be a good start :-)

Bye,

Andreas

Andreas Flack schrieb:
> P.S.: Furthermore, there is a but in the is_locked logic:
>
> is_locked (from line 758):
>
> // Object hasn't been marked to be edited
> if ($this->get('locked') === 0)
> {
> // Make sure locker is empty too
> if ($this->get('locker') != '')
> {
> $this->set('locker', '');
>
> get (from line 161):
> if (! array_key_exists($key, $this->_cache))
> {
> $this->_retrieve_value($key);
> }
>
>
> _retrieve_value (from line 393):
>
> // Person properties
> case 'creator':
> case 'revisor':
> case 'locker':
> case 'approver':
> case 'authors':
> $value = $this->object->metadata->$key;
> if (!$value)
> {
> // Fall back to "Midgard admin" if person is not found
> $value = 1;
> }
> break;
>
> So $this->get('locker') will never return '', but 1 instead, causing the
> object to be needlessly updated each time is_locked() is called.
>
> If you move case 'locker' up to the Time-based properties, this goes
> away. but I haven't really checked if it breaks anything else.
>
>
> Bye,
>
> Andreas
>
>
> Andreas Flack schrieb:
>> Hi!
>>
>> after updating MidCOM to the latest version, I noticed some strange
>> behavior in my application. It turned out that this came from the fact
>> that locking an object triggers the _on_updated functions of the
>> respective DBA classes, i.e. as soon as you click on "edit" for any
>> object, the pre and post update routines are executed.
>>
>> Personally, I find this rather unintuitive, as locking is not really
>> an update. So I was wondering: Is there a way to disable the execution
>> of the _on_updated and _on_updating functions for locking?
>> Alternatively, I'm thinking about doing something like
>>
>> function _on_updated()
>> {
>> if ($this->is_locked())
>> {
>> return true;
>> }
>> // otherwise, do your thing
>> }
>>
>> But I'm unsure if I could shoot myself in the foot with that (i.e.
>> could there be a timing problem in that the object gets saved without
>> unlocking it?)
>>
>>
>> Bye,
>>
>> Andreas
>> _______________________________________________
>> dev mailing list
>> dev@lists.midgard-project.org
>> http://lists.midgard-project.org/mailman/listinfo/dev
>
> _______________________________________________
> dev mailing list
> dev@lists.midgard-project.org
> http://lists.midgard-project.org/mailman/listinfo/dev

_______________________________________________
dev mailing list
dev@lists.midgard-project.org
http://lists.midgard-project.org/mailman/listinfo/dev

Reply
Report abuse

Andreas Flack

Re: [midgard-dev] lock() and _on_updated()

Sun July 20 2008 22:45:03 UTC

Hi!

Turns out I was wrong yet again. Instead of

(click "edit")
1: is_locked() (produces one update to locker if the object is unlocked)
2: lock() (locker is set)
3: lock() (locked is set)
(click "save")
4: update() (data is written)
5: unlock() (locker is unset)
6: unlock() (locked is unset)

The current implementation does:

(click "edit")
1: is_locked() (produces one update to locker if the object is unlocked)
2: lock() (locker is set)
3: lock() (locked is set)
(click "save")
4: unlock() (locker is unset)
5: unlock() (locked is unset)
6: update() (data is written)

which is of course even more messed up, since the object is unlocked
before it is actually updated (and even before the inputs are validated).

In case you're wondering: I have a conditional redirect in the
_on_updated function, and in the current implementation this causes the
changes to disappear magically each time you click "save"

Bye,

Andreas

Andreas Flack schrieb:
> Hi!
>
> Sorry for spamming, but the saga continues :-)
>
> As mentioned in my previous post, there is a bug which causes is_locked
> to update the object each time it is called, which until recently also
> produced an RCS revision, thus partly explaining the multiple revisions
> I saw each time I edited something.
>
> Since locking an object (even with the abovementioned bug removed)
> triggers the _on_updating and _on_updated handlers to execute, I added
> this to all the relevant functions:
>
>
> function _on_updated()
> {
> if ($this->metadata->locked)
> {
> return;
> }
>
> The theory was that _on_updated should only be executed when a DM2 lock
> is released, i.e. after the saving is complete. Of course, this is not
> perfect, since it could happen that there is a leftover lock, and the
> object is updated with the normal update() call, i.e. without DM2
> involved. Like I wrote in my first post, the best solution would be if
> locking and unlocking wouldn't trigger the event handlers at all. The
> second best thing would be if there was a global config option to
> disable locking altogether.
>
> But apart from that, I have one more problem. The code above doesn't
> even perform it's basic task, because it turns out that each locking and
> unlocking operation performs two updates, because locker and locked are
> written separately. So one edit with DM2 updates the object five or six
> times:
>
> 1: is_locked() (produces one update to locker if the object is unlocked)
> 2: lock() (locker is set)
> 3: lock() (locked is set)
> 4: update() (data is written)
> 5: unlock() (locker is unset)
> 6: unlock() (locked is unset)
>
> So the above code has to look like this:
>
> function _on_updated()
> if ($this->metadata->locked || $this->metadata->locker == '')
> {
> return;
> }
>
> Which is of course quite awkward.
>
> So I was wondering: Is there any real reason why locker and locked are
> written separately? If the locking was atomic, db i/o would be reduced
> and the bug in is_locked would disappear as well (because it only occurs
> in a consistency check when the object is unlocked and the function
> thinks that locker is still set). The above would then look like this:
>
> 1: lock() (locker and locked are set)
> 2: update() (data is written)
> 3: unlock() (locker and locked are unset)
>
> Maybe this could even be further optimized by removing the lock in the
> update step:
>
> 1: lock() (locker and locked are set)
> 2: update() (locker and locked are unset, data is written)
>
>
> But reducing the six updates to three would be a good start :-)
>
>
> Bye,
>
> Andreas
>
> Andreas Flack schrieb:
>> P.S.: Furthermore, there is a but in the is_locked logic:
>>
>> is_locked (from line 758):
>>
>> // Object hasn't been marked to be edited
>> if ($this->get('locked') === 0)
>> {
>> // Make sure locker is empty too
>> if ($this->get('locker') != '')
>> {
>> $this->set('locker', '');
>>
>> get (from line 161):
>> if (! array_key_exists($key, $this->_cache))
>> {
>> $this->_retrieve_value($key);
>> }
>>
>>
>> _retrieve_value (from line 393):
>>
>> // Person properties
>> case 'creator':
>> case 'revisor':
>> case 'locker':
>> case 'approver':
>> case 'authors':
>> $value = $this->object->metadata->$key;
>> if (!$value)
>> {
>> // Fall back to "Midgard admin" if person is not
>> found
>> $value = 1;
>> }
>> break;
>>
>> So $this->get('locker') will never return '', but 1 instead, causing
>> the object to be needlessly updated each time is_locked() is called.
>>
>> If you move case 'locker' up to the Time-based properties, this goes
>> away. but I haven't really checked if it breaks anything else.
>>
>>
>> Bye,
>>
>> Andreas
>>
>>
>> Andreas Flack schrieb:
>>> Hi!
>>>
>>> after updating MidCOM to the latest version, I noticed some strange
>>> behavior in my application. It turned out that this came from the
>>> fact that locking an object triggers the _on_updated functions of the
>>> respective DBA classes, i.e. as soon as you click on "edit" for any
>>> object, the pre and post update routines are executed.
>>>
>>> Personally, I find this rather unintuitive, as locking is not really
>>> an update. So I was wondering: Is there a way to disable the
>>> execution of the _on_updated and _on_updating functions for locking?
>>> Alternatively, I'm thinking about doing something like
>>>
>>> function _on_updated()
>>> {
>>> if ($this->is_locked())
>>> {
>>> return true;
>>> }
>>> // otherwise, do your thing
>>> }
>>>
>>> But I'm unsure if I could shoot myself in the foot with that (i.e.
>>> could there be a timing problem in that the object gets saved without
>>> unlocking it?)
>>>
>>>
>>> Bye,
>>>
>>> Andreas
>>> _______________________________________________
>>> dev mailing list
>>> dev@lists.midgard-project.org
>>> http://lists.midgard-project.org/mailman/listinfo/dev
>>
>> _______________________________________________
>> dev mailing list
>> dev@lists.midgard-project.org
>> http://lists.midgard-project.org/mailman/listinfo/dev
>
> _______________________________________________
> dev mailing list
> dev@lists.midgard-project.org
> http://lists.midgard-project.org/mailman/listinfo/dev

_______________________________________________
dev mailing list
dev@lists.midgard-project.org
http://lists.midgard-project.org/mailman/listinfo/dev

Reply
Report abuse

Andreas Flack

Re: [midgard-dev] lock() and _on_updated()

Tue July 22 2008 10:35:05 UTC

Hi!

I investigated a bit further and patched some of the problems of the
locking mechanism:

- I created a new _set_multiple method to write locker and locked at the
same time (should be used for approver/approved as well). This reduces
the number of updates on locking and unlocking and also solves the
is_locked() bug.

- I changed unlock to take one parameter $soft_unlock, which changes the
metadata properties without writing them back to the DB. If the DM2
simple controller is changed to use unlock(true), the unlock happens (or
fails) together with the main object update.

The effect of these changes is that only one object update is performed
when the editing forms are loaded (by lock) and one when the form is
saved (update and unlock). If there are no objections, I'd like to
commit this soon.

The original problem remains, however: _on_updating and _on_updated are
still triggered when the lock is created. I'm open to suggestions here :-)

Bye,

Andreas

Here's the diff:

Index: MidCOM_2_8/midcom.core/midcom/helper/metadata.php
===================================================================
--- MidCOM_2_8/midcom.core/midcom/helper/metadata.php (Revision 16940)
+++ MidCOM_2_8/midcom.core/midcom/helper/metadata.php (Arbeitskopie)
@@ -239,6 +239,52 @@
}

/**
+ * Frontend for setting a single metadata option
+ *
+ * @param string $key The key to set.
+ * @param mixed $value The value to set.
+ */
+ function set ($key, $value)
+ {
+ $return = false;
+ if ($this->_set_property($key, $value))
+ {
+ $return = $this->object->update();
+ // Update the corresponding cache variable
+ $this->on_update($key);
+ }
+ return $return;
+ }
+
+ /**
+ * Frontend for setting multiple metadata options
+ *
+ * @param Array $properties Array of key => value properties.
+ */
+ function set_multiple ($properties)
+ {
+ $return = false;
+ foreach ($properties as $key => $value)
+ {
+ if (!$this->_set_property($key, $value))
+ {
+ return false;
+ // this will exit
+ }
+ }
+ if ($this->object->update())
+ {
+ $return = true;
+ // Update the corresponding cache variables
+ foreach ($properties as $key => $value)
+ {
+ $this->on_update($key);
+ }
+ }
+ return $return;
+ }
+
+ /**
* Directly set a metadata option.
*
* The passed value will be stored using the follow transformations:
@@ -253,7 +299,7 @@
* @param string $key The key to set.
* @param mixed $value The value to set.
*/
- function set ($key, $value)
+ function _set_property ($key, $value)
{
// Store the RCS mode
$rcs_mode = $this->object->_use_rcs;
@@ -294,7 +340,7 @@
case 'navnoentry':
case 'score':
$this->object->metadata->$key = $value;
- $value = $this->object->update();
+ $value = true;
break;

// Fall-back for non-core properties
@@ -303,8 +349,6 @@
break;
}

- // Update the corresponding cache variable
- $this->on_update($key);
debug_pop();

// Return the original RCS mode
@@ -314,6 +358,7 @@
}

+
/**
* This is the update event handler for the Metadata system. It
must be called
* whenever metadata changes to synchronize the various
backwards-compatibility
@@ -758,21 +803,13 @@
// Object hasn't been marked to be edited
if ($this->get('locked') === 0)
{
- // Make sure locker is empty too
- if ($this->get('locker') != '')
- {
- $this->set('locker', '');
- }
-
return false;
}

if (($this->get('locked') +
($GLOBALS['midcom_config']['metadata_lock_timeout'] * 60)) < time())
{
// lock expired, explicitly clear lock
- $this->set('locked', 0);
- $this->set('locker', '');
-
+ $this->set_multiple(Array('locked' => 0, 'locker' => ''));
return false;
}

@@ -805,20 +842,16 @@

if (!$user)
{
- $this->set('locker', $_MIDCOM->auth->user->guid);
+ $user = $_MIDCOM->auth->user->guid;
}
- else
- {
- $this->set('locker', $user);
- }
-
- // Update failed, return false
- if (!$this->set('locked', time() + $timeout * 60))
- {
- return false;
- }
-
- return true;
+
+ $lock = Array
+ (
+ 'locker' => $user,
+ 'locked' => time() + $timeout * 60
+ );
+
+ return $this->set_multiple($lock);
}

/**
@@ -841,17 +874,24 @@
* Unlock the object
*
* @access public
- * @param boolean Indicating success
+ * @param boolean $soft_unlock If this is true, the changes are not
written to disk
+ * @return boolean Indicating success
*/
- function unlock()
+ function unlock($soft_unlock = false)
{
if (!$this->can_unlock())
{
return false;
}
+
+ if ($soft_unlock)
+ {
+ $this->object->metadata->locked = 0;
+ $this->object->metadata->locker = '';
+ return true;
+ }

- if ( !$this->set('locker', '')
- || !$this->set('locked', ''))
+ if (!$this->set_multiple(Array('locked' => 0, 'locker' => '')))
{
return false;
}
@@ -859,4 +899,4 @@
return true;
}
}
-?>
\ No newline at end of file
+?>
Index: MidCOM_2_8/midcom.helper.datamanager2/controller/simple.php
===================================================================
--- MidCOM_2_8/midcom.helper.datamanager2/controller/simple.php
(Revision 16940)
+++ MidCOM_2_8/midcom.helper.datamanager2/controller/simple.php
(Arbeitskopie)
@@ -93,7 +93,7 @@
&& ( $result === 'save'
|| $result === 'cancel'))
{
- $metadata->unlock();
+ $metadata->unlock(true);
}
// or set it, if needed
elseif ($this->lock_object

_______________________________________________
dev mailing list
dev@lists.midgard-project.org
http://lists.midgard-project.org/mailman/listinfo/dev

Reply
Report abuse

Henri Bergius

Re: [midgard-dev] lock() and _on_updated()

Fri July 25 2008 14:00:04 UTC

Hi,

On Tue, Jul 22, 2008 at 1:30 PM, Andreas Flack
<flack@contentcontrol-berlin.de> wrote:
> The original problem remains, however: _on_updating and _on_updated are
> still triggered when the lock is created. I'm open to suggestions here :-)

I've been thinking a bit about this too, and come to the idea that we
need new MgdSchema methods:

* lock
* unlock
* approve
* unapprove

So that we can keep update() in use for only actual data updates.

Piotras?

> Andreas

/Henri

--
Henri Bergius
Motorcycle Adventures and Free Software
http://bergie.iki.fi/

Skype: henribergius
Jabber: henri.bergius@gmail.com
Jaiku: http://bergie.jaiku.com/
_______________________________________________
dev mailing list
dev@lists.midgard-project.org
http://lists.midgard-project.org/mailman/listinfo/dev

Reply
Report abuse

Piotr Pokora

Re: [midgard-dev] lock() and _on_updated()

Mon July 28 2008 08:45:03 UTC

Henri Bergius writes:
> Hi,

Hi!

>> The original problem remains, however: _on_updating and _on_updated are
>> still triggered when the lock is created. I'm open to suggestions here :-)
>
> I've been thinking a bit about this too, and come to the idea that we
> need new MgdSchema methods:
>
> * lock
> * unlock
> * approve
> * unapprove

Additional select we need to make during update, delete and purge
shouldn't be a problem as we can use the one which is executed to find
duplicates in tree ( at least for update ). Question is when lock should
be ignored.
And who could unlock object if the one is already locked.
And what about approve? Should it be simple switch or like legacy one
datetime toggle?

Piotras
_______________________________________________
dev mailing list
dev@lists.midgard-project.org
http://lists.midgard-project.org/mailman/listinfo/dev

Reply
Report abuse

Henri Bergius

Re: [midgard-dev] lock() and _on_updated()

Mon July 28 2008 08:55:03 UTC

Hi,

On Mon, Jul 28, 2008 at 11:42 AM, Piotr Pokora <piotrek.pokora@gmail.com> wrote:
> Additional select we need to make during update, delete and purge
> shouldn't be a problem as we can use the one which is executed to find
> duplicates in tree ( at least for update ). Question is when lock should
> be ignored.
> And who could unlock object if the one is already locked.
> And what about approve? Should it be simple switch or like legacy one
> datetime toggle?

I think the methods should just update the appropriate metadata
properties (locked, locker, approved, approver). The checks are better
done on application level, as they can be a bit different depending on
the use case. For instance, MidCOM-level locks are much simpler than
WebDAV's token-based locks.

> Piotras

/Bergie

--
Henri Bergius
Motorcycle Adventures and Free Software
http://bergie.iki.fi/

Skype: henribergius
Jabber: henri.bergius@gmail.com
Jaiku: http://bergie.jaiku.com/
_______________________________________________
dev mailing list
dev@lists.midgard-project.org
http://lists.midgard-project.org/mailman/listinfo/dev

Reply
Report abuse

Piotr Pokora

Re: [midgard-dev] lock() and _on_updated()

Mon July 28 2008 09:00:04 UTC

Henri Bergius writes:
> Hi,

Hi!

>> And who could unlock object if the one is already locked.
>> And what about approve? Should it be simple switch or like legacy one
>> datetime toggle?
>
> I think the methods should just update the appropriate metadata
> properties (locked, locker, approved, approver). The checks are better
> done on application level, as they can be a bit different depending on
> the use case.

So you want only API shortcuts, without adding new logic to existing
methods?
If yes, please add new feature request at trac.

Piotras
_______________________________________________
dev mailing list
dev@lists.midgard-project.org
http://lists.midgard-project.org/mailman/listinfo/dev

Reply
Report abuse

Henri Bergius

Re: [midgard-dev] lock() and _on_updated()

Tue July 29 2008 13:55:03 UTC

Hi,

On Mon, Jul 28, 2008 at 11:57 AM, Piotr Pokora <piotrek.pokora@gmail.com> wrote:
> So you want only API shortcuts, without adding new logic to existing
> methods?
> If yes, please add new feature request at trac.

Here: http://trac.midgard-project.org/ticket/265

> Piotras

/Bergie

--
Henri Bergius
Motorcycle Adventures and Free Software
http://bergie.iki.fi/

Skype: henribergius
Jabber: henri.bergius@gmail.com
Jaiku: http://bergie.jaiku.com/
_______________________________________________
dev mailing list
dev@lists.midgard-project.org
http://lists.midgard-project.org/mailman/listinfo/dev

Reply
Report abuse