Skip to content
This repository has been archived by the owner on Oct 18, 2024. It is now read-only.

Added onOneServer option to short run commands #8

Merged
merged 7 commits into from
Jul 13, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,14 @@ Commands won't run whilst Laravel is in maintenance mode. If you would like to f
$shortSchedule->command('artisan-command')->everySecond()->runInMaintenanceMode();
```

### Running Tasks On One Server

Limit commands to only run on one server at a time.

```php
$shortSchedule->command('artisan-command')->everySecond()->onOneServer();
```

## Events

Executing any code when responding to these events is blocking. If your code takes a long time to execute, all short scheduled jobs will be delayed. We highly recommend to put any code you wish to execute in response to these events on a queue.
Expand Down
1 change: 1 addition & 0 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
],
"require": {
"php": "^7.4",
"illuminate/cache": "^7.0",
"react/event-loop": "^1.1",
"spatie/temporary-directory": "^1.2"
},
Expand Down
19 changes: 19 additions & 0 deletions src/PendingShortScheduleCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ class PendingShortScheduleCommand

protected bool $allowOverlaps = true;

protected bool $onOneServer = false;

protected bool $evenInMaintenanceMode = false;

protected array $constraints = [];
Expand Down Expand Up @@ -71,6 +73,13 @@ public function shouldRun(): bool
return ! $shouldNotRun;
}

public function onOneServer(): self
{
$this->onOneServer = true;

return $this;
}

public function between(string $startTime, string $endTime): self
{
$this->constraints[] = new BetweenConstraint($startTime, $endTime);
Expand Down Expand Up @@ -98,4 +107,14 @@ public function when(Closure $closure): self

return $this;
}

public function getOnOneServer()
lionslair marked this conversation as resolved.
Show resolved Hide resolved
{
return $this->onOneServer;
}

public function cacheName()
{
return 'framework'.DIRECTORY_SEPARATOR.'schedule-'.sha1($this->frequencyInSeconds.$this->command);
}
}
23 changes: 22 additions & 1 deletion src/ShortScheduleCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Spatie\ShortSchedule;

use Illuminate\Support\Facades\App;
use Illuminate\Support\Facades\Cache;
use Spatie\ShortSchedule\Events\ShortScheduledTaskStarted;
use Spatie\ShortSchedule\Events\ShortScheduledTaskStarting;
use Symfony\Component\Process\Process;
Expand Down Expand Up @@ -50,9 +51,29 @@ public function isRunning(): bool
}

public function run(): void
{
$this->pendingShortScheduleCommand->getOnOneServer() ? $this->processOnOneServer() : $this->processCommand() ;
}

private function processOnOneServer()
lionslair marked this conversation as resolved.
Show resolved Hide resolved
{
if (Cache::missing($this->pendingShortScheduleCommand->cacheName())) {
lionslair marked this conversation as resolved.
Show resolved Hide resolved
Cache::add($this->pendingShortScheduleCommand->cacheName(), true, 60);
Copy link
Member

Choose a reason for hiding this comment

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

Why 60? Does the Laravel framework code also uses this value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The framework was using 3600 but I thought 60 seconds as this whole package is about sub minute tasks. I was thinking it should use
$this->pendingShortScheduleCommand->frequencyInSeconds()

so the expiration lasts only as long as the time between checks. However this should also depend on if you care about overlaps or not.

If it's too short could be an issue for some and same if it's too long. I assume the frameworks assumption no task will run for more than an hour.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's leave it at 60.

Should we also have to clean up this file when starting the loop? Right now, if we were to abort the loop at the wrong time, the file would stay, keeping to lock longer than it needs to be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean in the situation the short-schedule:run is killed or dies then when it starts again that the command will be stuck because if the cache key is set? I have not experienced this. I am not 100% sure the running command actually terminates at the same time as as the loop.

So short-schedule:run can stop and I think the process that was running can still run and finish. It just will not be called again.

Worst case the key will expire in 1 minute. So in theory it should start running on one sever after 60 seconds in worst case scenario.


$this->processCommand();

while ($this->process->isRunning()) {
lionslair marked this conversation as resolved.
Show resolved Hide resolved
// waiting for process to finish before clearing the cache item
}

Cache::forget($this->pendingShortScheduleCommand->cacheName());
}
}

private function processCommand()
{
$commandString = $this->pendingShortScheduleCommand->command;
$this->process = Process::fromShellCommandline($this->pendingShortScheduleCommand->command);
$this->process = Process::fromShellCommandline($commandString);

event(new ShortScheduledTaskStarting($commandString, $this->process));
$this->process->start();
Expand Down
18 changes: 18 additions & 0 deletions tests/Feature/ShortScheduleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Spatie\ShortSchedule\Tests\Feature;

use Illuminate\Support\Facades\Cache;
use Spatie\ShortSchedule\ShortSchedule;
use Spatie\ShortSchedule\Tests\TestCase;
use Spatie\ShortSchedule\Tests\TestClasses\TestKernel;
Expand Down Expand Up @@ -113,4 +114,21 @@ public function it_will_run_whilst_in_maintenance_mode()

$this->artisan('up')->expectsOutput('Application is now live.')->assertExitCode(0);
}

/** @test **/
public function it_will_run_command_on_one_server()
{
TestKernel::registerShortScheduleCommand(
fn (ShortSchedule $shortSchedule) => $shortSchedule
->exec("echo 'called' >> '{$this->getTempFilePath()}'")
->everySeconds(0.05)
->onOneServer()
);

$this
->runShortScheduleForSeconds(0.14)
->assertTempFileContains('called', 2);

$this->assertTrue(Cache::has('framework'.DIRECTORY_SEPARATOR.'schedule-'.sha1('0.05'."echo 'called' >> '{$this->getTempFilePath()}'")));
}
}