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

[5.4] Cache based Schedule overlap locking #16196

Merged
merged 2 commits into from Oct 31, 2016

Conversation

Projects
None yet
5 participants
@themsaid
Copy link
Member

commented Oct 31, 2016

This is an attempt to solve the following issue: laravel/ideas#69

Instead of using the file system to store a lock file, we use the cache store to allow any server to lock/check scheduled events.

@taylorotwell taylorotwell merged commit 022dbb3 into laravel:master Oct 31, 2016

2 checks passed

continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexbowers

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2016

Any chance of a backport to 5.3 for this?

@themsaid

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2016

@alexbowers isn't this breaking? an overlap might occur between update.

@alexbowers

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2016

What do you mean between update?

@themsaid

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2016

I meant, while upgrading, the new check will look for a cache key while the first event used a lock file.

@alexbowers

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2016

That is the same surely whether you upgrade to 5.3.x or 5.4?

The release process will have a breaking change between them. Or is the issue just because it is a patch version update?

Any reasonable release process on multiple servers will include zero downtime, which would indicate that the release goes live on all servers at the same time, or that servers are taken out of circulation whilst the upgrade is going on, until all servers are upgraded and at the same state.

After the upgrade period have completed, all servers will match, so having the mutex in a file, or in cache shouldn't make any difference, since they'd be using the same checking code.

Any release process on a single server shouldn't matter, since there is only one cronjob to run at any one time anyway, so the mutex is largely meaningless here during the upgrade process.

Or am i misunderstanding the issue?

@themsaid

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2016

  • Event 1 running
  • Lock file was created
  • Upgrade finished
  • Event 1 running again, checking for a cache key
  • no cache key found, so event 1 runs while it's still running in the first process
@alexbowers

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2016

Wouldn't this be an issue in a 5.3.x to 5.4 upgrade too?

In that case, A fix for that would be that if a cache key isn't found, to check for the lock file. This would make the upgrade process completely BC, and a later patch can remove the lock file, to make it not perform unnecessary actions (Whether 5.5, or 5.4.x)

@@ -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());

This comment has been minimized.

Copy link
@alexbowers

alexbowers Nov 1, 2016

Contributor

@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());

This comment has been minimized.

Copy link
@alexbowers

alexbowers Nov 1, 2016

Contributor

Perhaps the unlink method too.

@tillkruss

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

@themsaid: What happens if the cache is flushed? Wouldn't make even more sense use a database table?

@Einkoro

This comment has been minimized.

Copy link

commented Aug 10, 2017

Any chance of this being back-ported to 5.1 LTS?

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.