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
Inhibit concurrent execution of Update::updateToCurrentVersion() #6457
Conversation
4aa7f7f
to
42a81b0
Compare
It turns out that TravisCI supports PHP Semaphores 😉 |
@@ -198,7 +199,9 @@ public function handleAutomaticUpdates() | |||
$installed = $config->get('concrete.version_db_installed'); | |||
$core = $config->get('concrete.version_db'); | |||
if ($installed < $core) { | |||
Update::updateToCurrentVersion(); | |||
$this->make(MutexInterface::class)->execute(Update::MUTEX_KEY, function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should we do if the Update::updateToCurrentVersion()
is already in progress?
With this code we have an uncaught Concrete\Core\System\Mutex\MutexBusyException
exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make it so that the maintenance mode view is rendered if this is the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Here it is: #6480
This is needed because someone may set the concrete5 temporary directory to point to a folder shared across multiple concrete5 instances.
If we use a custom algorithm to determine the ID of a semaphore, there's a (remote) possibility to end up using a semaphore defined by other system apps.
The temporary directory may be shared across multiple concrete5 instances, and we need to be sure that the mutex are instance-specific.
On high-traffic sites, we may have that
Update::updateToCurrentVersion()
is executed simultaneously more that one time, leading to unexpected results.What about protecting us from that by using mutex?
I added two mutex implementations here: one based on PHP Semaphores (it requires that PHP is compiled with the
--enable-sysvsem
option), and another one based on file locks.I also included a test case to check that these mutex works. I don't think that TravisCI supports Semaphores by default, but I tested with a PHP 7.0 compiled with
--enable-sysvsem
and test passes.