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] Sync Event and CallbackEvent withoutOverlapping functions #20389

Merged
merged 1 commit into from
Aug 2, 2017
Merged

[5.4] Sync Event and CallbackEvent withoutOverlapping functions #20389

merged 1 commit into from
Aug 2, 2017

Conversation

laurencei
Copy link
Contributor

The signatures for withoutOverlapping() in Event and CallbackEvent are different.

This sync's up the Event to match CallbackEvent, allowing people to set a cache expiration time on Event in the same way as a CallbackEvent.

Non-breaking change - will keep current behavior if no value is set.

@taylorotwell
Copy link
Member

taylorotwell commented Aug 2, 2017

I'm not sure this is actually needed / required for non-callback events? I'm not sure it is even used?

@laurencei
Copy link
Contributor Author

laurencei commented Aug 2, 2017

@taylorotwell - it gives flexibility.

If your command dies (i.e. the server reboots, PHP crashes etc etc) - your mutex remains for 24 hours because the after callbacks are never run.

This PR allows some control to set a shorter (or longer) mutex if you want.

I'm not sure it is even used?

If you fire withoutOverlapping() - a mutex is definitely created. Especially if you combine it with runInBackground()

@taylorotwell
Copy link
Member

But where is expiresAt used by this class?

@laurencei
Copy link
Contributor Author

laurencei commented Aug 2, 2017

It is used by the Mutex. The Mutex gets passed $this - and then the mutex calls expiresAt.

https://github.com/laravel/framework/blob/5.4/src/Illuminate/Console/Scheduling/CacheMutex.php#L36

So right now all withoutOverlapping() on Event use $expiresAt = 1440 - as set as the default in the Event class.

@taylorotwell taylorotwell merged commit c04ee7a into laravel:5.4 Aug 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants