Skip to content

[5.4] Sync Event and CallbackEvent withoutOverlapping functions#20389

Merged
taylorotwell merged 1 commit intolaravel:5.4from
laurencei:patch-2
Aug 2, 2017
Merged

[5.4] Sync Event and CallbackEvent withoutOverlapping functions#20389
taylorotwell merged 1 commit intolaravel:5.4from
laurencei:patch-2

Conversation

@laurencei
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

But where is expiresAt used by this class?

@laurencei
Copy link
Copy Markdown
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