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.

Sign up for GitHub

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

Jump to bottom

[5.4] Cache based Schedule overlap locking #16196

Merged
merged 2 commits into from
Oct 31, 2016
Merged

[5.4] Cache based Schedule overlap locking #16196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef1e133
Cache based Schedule overlap locking
themsaid Oct 31, 2016
022dbb3
re order arguments
themsaid Oct 31, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions src/Illuminate/Console/Scheduling/CallbackEvent.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use LogicException;
use InvalidArgumentException;
use Illuminate\Contracts\Container\Container;
use Illuminate\Contracts\Cache\Repository as Cache;

class CallbackEvent extends Event
{
Expand All @@ -15,6 +16,13 @@ class CallbackEvent extends Event
*/
protected $callback;

/**
* The cache store implementation.
*
* @var \Illuminate\Contracts\Cache\Repository
*/
protected $cache;

/**
* The parameters to pass to the method.
*
Expand All @@ -26,19 +34,21 @@ class CallbackEvent extends Event
* Create a new event instance.
*
* @param string $callback
* @param \Illuminate\Contracts\Cache\Repository $cache
* @param array $parameters
* @return void
*
* @throws \InvalidArgumentException
*/
public function __construct($callback, array $parameters = [])
public function __construct($callback, Cache $cache, array $parameters = [])
{
if (! is_string($callback) && ! is_callable($callback)) {
throw new InvalidArgumentException(
'Invalid scheduled callback event. Must be string or callable.'
);
}

$this->cache = $cache;
$this->callback = $callback;
$this->parameters = $parameters;
}
Expand All @@ -54,7 +64,7 @@ public function __construct($callback, array $parameters = [])
public function run(Container $container)
{
if ($this->description) {
touch($this->mutexPath());
$this->cache->put($this->mutexName(), true, 1440);
}

try {
Expand All @@ -76,7 +86,7 @@ public function run(Container $container)
protected function removeMutex()
{
if ($this->description) {
@unlink($this->mutexPath());
$this->cache->forget($this->mutexName());
}
}

Expand All @@ -96,18 +106,18 @@ public function withoutOverlapping()
}

return $this->skip(function () {
return file_exists($this->mutexPath());
return $this->cache->has($this->mutexName());
});
}

/**
* Get the mutex path for the scheduled command.
* Get the mutex name for the scheduled command.
*
* @return string
*/
protected function mutexPath()
protected function mutexName()
{
return storage_path('framework/schedule-'.sha1($this->description));
return 'framework/schedule-'.sha1($this->description);
}

/**
Expand Down
38 changes: 24 additions & 14 deletions src/Illuminate/Console/Scheduling/Event.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use Symfony\Component\Process\Process;
use Symfony\Component\Process\ProcessUtils;
use Illuminate\Contracts\Container\Container;
use Illuminate\Contracts\Cache\Repository as Cache;

class Event
{
Expand All @@ -21,6 +22,13 @@ class Event
*/
public $command;

/**
* The cache store implementation.
*
* @var \Illuminate\Contracts\Cache\Repository
*/
protected $cache;

/**
* The cron expression representing the event's frequency.
*
Expand Down Expand Up @@ -123,10 +131,12 @@ class Event
* Create a new event instance.
*
* @param string $command
* @param \Illuminate\Contracts\Cache\Repository $cache
* @return void
*/
public function __construct($command)
public function __construct($command, Cache $cache)
{
$this->cache = $cache;
$this->command = $command;
$this->output = $this->getDefaultOutput();
}
Expand All @@ -149,11 +159,19 @@ protected function getDefaultOutput()
*/
public function run(Container $container)
{
if ($this->withoutOverlapping) {
$this->cache->put($this->mutexName(), true, 1440);
}

if (! $this->runInBackground) {
$this->runCommandInForeground($container);
} else {
$this->runCommandInBackground();
}

if ($this->withoutOverlapping) {
$this->cache->forget($this->mutexName());
}
}

/**
Expand Down Expand Up @@ -222,27 +240,19 @@ public function buildCommand()

$redirect = $this->shouldAppendOutput ? ' >> ' : ' > ';

if ($this->withoutOverlapping) {
if (windows_os()) {
$command = '(echo \'\' > "'.$this->mutexPath().'" & '.$this->command.' & del "'.$this->mutexPath().'")'.$redirect.$output.' 2>&1 &';
} else {
$command = '(touch '.$this->mutexPath().'; '.$this->command.'; rm '.$this->mutexPath().')'.$redirect.$output.' 2>&1 &';
}
} else {
$command = $this->command.$redirect.$output.' 2>&1 &';
}
$command = $this->command.$redirect.$output.' 2>&1 &';

return $this->user && ! windows_os() ? 'sudo -u '.$this->user.' -- sh -c \''.$command.'\'' : $command;
}

/**
* Get the mutex path for the scheduled command.
* Get the mutex name for the scheduled command.
*
* @return string
*/
protected function mutexPath()
protected function mutexName()
{
return storage_path('framework'.DIRECTORY_SEPARATOR.'schedule-'.sha1($this->expression.$this->command));
return 'framework'.DIRECTORY_SEPARATOR.'schedule-'.sha1($this->expression.$this->command);
}

/**
Expand Down Expand Up @@ -719,7 +729,7 @@ public function withoutOverlapping()
$this->withoutOverlapping = true;

return $this->skip(function () {
return file_exists($this->mutexPath());
return $this->cache->has($this->mutexName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@themsaid I believe this is the only line that would need to change to make it fully BC. Something like:

return $this->cache->has($this->mutexName()) || file_exists($this->mutexPath());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the unlink method too.

});
}

Expand Down
23 changes: 21 additions & 2 deletions src/Illuminate/Console/Scheduling/Schedule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,28 @@
use Illuminate\Container\Container;
use Symfony\Component\Process\ProcessUtils;
use Symfony\Component\Process\PhpExecutableFinder;
use Illuminate\Contracts\Cache\Repository as Cache;

class Schedule
{
/**
* The cache store implementation.
*
* @var \Illuminate\Contracts\Cache\Repository
*/
protected $cache;

/**
* Create a new event instance.
*
* @param \Illuminate\Contracts\Cache\Repository $cache
* @return void
*/
public function __construct(Cache $cache)
{
$this->cache = $cache;
}

/**
* All of the events on the schedule.
*
Expand All @@ -24,7 +43,7 @@ class Schedule
*/
public function call($callback, array $parameters = [])
{
$this->events[] = $event = new CallbackEvent($callback, $parameters);
$this->events[] = $event = new CallbackEvent($callback, $this->cache, $parameters);

return $event;
}
Expand Down Expand Up @@ -62,7 +81,7 @@ public function exec($command, array $parameters = [])
$command .= ' '.$this->compileParameters($parameters);
}

$this->events[] = $event = new Event($command);
$this->events[] = $event = new Event($command, $this->cache);

return $event;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Illuminate/Foundation/Console/Kernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public function __construct(Application $app, Dispatcher $events)
protected function defineConsoleSchedule()
{
$this->app->instance(
'Illuminate\Console\Scheduling\Schedule', $schedule = new Schedule
'Illuminate\Console\Scheduling\Schedule', $schedule = new Schedule($this->app['cache.store'])
);

$this->schedule($schedule);
Expand Down
15 changes: 12 additions & 3 deletions tests/Console/ConsoleEventSchedulerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,15 @@

class ConsoleEventSchedulerTest extends PHPUnit_Framework_TestCase
{
public function setUp()
{
parent::setUp();

\Illuminate\Container\Container::getInstance()->instance(
'Illuminate\Console\Scheduling\Schedule', $this->schedule = new Schedule(m::mock('Illuminate\Contracts\Cache\Repository'))
);
}

public function tearDown()
{
m::close();
Expand All @@ -15,7 +24,7 @@ public function testExecCreatesNewCommand()
$escape = '\\' === DIRECTORY_SEPARATOR ? '"' : '\'';
$escapeReal = '\\' === DIRECTORY_SEPARATOR ? '\\"' : '"';

$schedule = new Schedule;
$schedule = $this->schedule;
$schedule->exec('path/to/command');
$schedule->exec('path/to/command -f --foo="bar"');
$schedule->exec('path/to/command', ['-f']);
Expand All @@ -40,7 +49,7 @@ public function testCommandCreatesNewArtisanCommand()
{
$escape = '\\' === DIRECTORY_SEPARATOR ? '"' : '\'';

$schedule = new Schedule;
$schedule = $this->schedule;
$schedule->command('queue:listen');
$schedule->command('queue:listen --tries=3');
$schedule->command('queue:listen', ['--tries' => 3]);
Expand All @@ -56,7 +65,7 @@ public function testCreateNewArtisanCommandUsingCommandClass()
{
$escape = '\\' === DIRECTORY_SEPARATOR ? '"' : '\'';

$schedule = new Schedule;
$schedule = $this->schedule;
$schedule->command(ConsoleCommandStub::class, ['--force']);

$events = $schedule->events();
Expand Down
36 changes: 18 additions & 18 deletions tests/Console/ConsoleScheduledEventTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public function testBasicCronCompilation()
$app->shouldReceive('isDownForMaintenance')->andReturn(false);
$app->shouldReceive('environment')->andReturn('production');

$event = new Event('php foo');
$event = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals('* * * * * *', $event->getExpression());
$this->assertTrue($event->isDue($app));
$this->assertTrue($event->skip(function () {
Expand All @@ -42,49 +42,49 @@ public function testBasicCronCompilation()
return true;
})->filtersPass($app));

$event = new Event('php foo');
$event = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals('* * * * * *', $event->getExpression());
$this->assertFalse($event->environments('local')->isDue($app));

$event = new Event('php foo');
$event = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals('* * * * * *', $event->getExpression());
$this->assertFalse($event->when(function () {
return false;
})->filtersPass($app));

$event = new Event('php foo');
$event = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals('*/5 * * * * *', $event->everyFiveMinutes()->getExpression());

$event = new Event('php foo');
$event = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals('0 0 * * * *', $event->daily()->getExpression());

$event = new Event('php foo');
$event = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals('0 3,15 * * * *', $event->twiceDaily(3, 15)->getExpression());

$event = new Event('php foo');
$event = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals('*/5 * * * 3 *', $event->everyFiveMinutes()->wednesdays()->getExpression());

$event = new Event('php foo');
$event = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals('0 * * * * *', $event->everyFiveMinutes()->hourly()->getExpression());

$event = new Event('php foo');
$event = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals('0 15 4 * * *', $event->monthlyOn(4, '15:00')->getExpression());

$event = new Event('php foo');
$event = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals('0 0 * * 1-5 *', $event->weekdays()->daily()->getExpression());

$event = new Event('php foo');
$event = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals('0 * * * 1-5 *', $event->weekdays()->hourly()->getExpression());

// chained rules should be commutative
$eventA = new Event('php foo');
$eventB = new Event('php foo');
$eventA = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$eventB = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals(
$eventA->daily()->hourly()->getExpression(),
$eventB->hourly()->daily()->getExpression());

$eventA = new Event('php foo');
$eventB = new Event('php foo');
$eventA = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$eventB = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals(
$eventA->weekdays()->hourly()->getExpression(),
$eventB->hourly()->weekdays()->getExpression());
Expand All @@ -97,11 +97,11 @@ public function testEventIsDueCheck()
$app->shouldReceive('environment')->andReturn('production');
Carbon::setTestNow(Carbon::create(2015, 1, 1, 0, 0, 0));

$event = new Event('php foo');
$event = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals('* * * * 4 *', $event->thursdays()->getExpression());
$this->assertTrue($event->isDue($app));

$event = new Event('php foo');
$event = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertEquals('0 19 * * 3 *', $event->wednesdays()->at('19:00')->timezone('EST')->getExpression());
$this->assertTrue($event->isDue($app));
}
Expand All @@ -113,7 +113,7 @@ public function testTimeBetweenChecks()
$app->shouldReceive('environment')->andReturn('production');
Carbon::setTestNow(Carbon::now()->startOfDay()->addHours(9));

$event = new Event('php foo');
$event = new Event('php foo', m::mock('Illuminate\Contracts\Cache\Repository'));
$this->assertTrue($event->between('8:00', '10:00')->filtersPass($app));
$this->assertTrue($event->between('9:00', '9:00')->filtersPass($app));
$this->assertFalse($event->between('10:00', '11:00')->filtersPass($app));
Expand Down
Loading