Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix EZP-23908: expiry.php race condition #1139

Merged
merged 1 commit into from Jan 26, 2015

Conversation

4 participants
@glye
Copy link
Member

glye commented Jan 20, 2015

Restore timestamps before saving, to reduce chance of race condition issues

This is the easy solution. The complete solution would be an asyncronous process with expiry request stored in db and a cron daemon which processes them at short intervals. I consider that overkill for legacy, which should not have new development.

Unknown: How much does this fix affect performance in cluster environments?

https://jira.ez.no/browse/EZP-23908

$modifiedTimestamps[$name] > $this->Timestamps[$name] )
{
$this->Timestamps[$name] = $modifiedTimestamps[$name];
$this->IsModified = true;

This comment has been minimized.

Copy link
@joaoinacio

joaoinacio Jan 20, 2015

Contributor

Nitpick: IMHO, something like the following would look a bit cleaner:

if ( $value > $this->getTimestamp( $name, 0 )
{
     $this->setTimestamp( $name, $value )
}

This comment has been minimized.

Copy link
@glye

glye Jan 20, 2015

Author Member

Fair enough, except that getTimestamp() is static :)

This comment has been minimized.

Copy link
@joaoinacio

joaoinacio Jan 20, 2015

Contributor

Doh indeed, good catch!
Well something like if ( !$this->hasTimestamp( $name ) || $value > $this->timestamp($name) ) ?

@glye

This comment has been minimized.

Copy link
Member Author

glye commented Jan 20, 2015

Re: Travis, I don't see how expiry.php can cause eZImageTypeRegression::tearDown() to fail. Bogus? Then again, messing with expiry can affect a lot of things...

@glye glye force-pushed the glye:ezp-23908_expiry_php_race_condition branch from 14c351e to bb40391 Jan 20, 2015

@joaoinacio

This comment has been minimized.

Copy link
Contributor

joaoinacio commented Jan 20, 2015

No idea about the previous travis fail either, very weird glitch...

Regarding the performance aspect, a possible improvement would be to lazy-load expiry data on the hasTimstamp() / getTimestamp() / timestamp() methods.
In theory, this could potentially reduce the fetch/restore by 1 in some cases?

In any case, this should pretty much resolve the issue - even if it's not 100% fail-safe (no additional locking, etc) I don't see any other solution that wouldn't involve a major rewrite/implementation...
So FYIW, +1

@glye

This comment has been minimized.

Copy link
Member Author

glye commented Jan 20, 2015

@joaoinacio Thanks. In what cases would lazy loading help?

@joaoinacio

This comment has been minimized.

Copy link
Contributor

joaoinacio commented Jan 20, 2015

@glye:
I imagine there are a few code paths were expiry data isn't used except for actually expiring content on shutdown?
In practice I have no idea what the percentage of the usage/requests actually needs expiry data, but lazy-loading would also have the potential benefit of reading "fresher" values...

@glye

This comment has been minimized.

Copy link
Member Author

glye commented Jan 20, 2015

@georgfranz Hi! This is not needed anymore, after the change suggested by @joaoinacio. isModified is set to false by the restore() call before the foreach, and set to true by setTimestamp().

@georgfranz

This comment has been minimized.

Copy link

georgfranz commented Jan 20, 2015

ah, ok! tx for explanation!

@@ -61,6 +61,24 @@ static function fetchData( $path )
*/
function store()
{
if ( !($this->IsModified) )

This comment has been minimized.

Copy link
@lolautruche

lolautruche Jan 26, 2015

Contributor

Inner parenthesis are not needed

@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Jan 26, 2015

+1

Fix EZP-23908: expiry.php race condition
Restore timestamps before saving, to reduce chance of race condition issues
Simplified redundant code
Removed unneeded parenthesis

@glye glye force-pushed the glye:ezp-23908_expiry_php_race_condition branch from bb40391 to aca99a1 Jan 26, 2015

glye added a commit that referenced this pull request Jan 26, 2015

Merge pull request #1139 from glye/ezp-23908_expiry_php_race_condition
Fix EZP-23908: expiry.php race condition

@glye glye merged commit c7caced into ezsystems:master Jan 26, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@glye glye deleted the glye:ezp-23908_expiry_php_race_condition branch Jan 26, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.