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

Question: Is there any way to run an invocable on demand while respecting PreventOverlapping? #96

Open
oltronix opened this issue Jul 2, 2019 · 13 comments
Assignees

Comments

@oltronix
Copy link

oltronix commented Jul 2, 2019

So lets say we have a moderately heavy job that imports something to our db every 20 minutes. Occasionally we are asked to run this manually because the 20 minutes wait is just to long, using Coravel we could then trigger this using the Queue, but if we're unlucky this would coincide with the scheduled run every twenty minutes, which would cause some issues in legacy exports we have. Can this overlap be detected and avoided?

As an alternative it would be nice if we could schedule a one off run of an invocable, like scheduler.Schedule().Once();
That way it could take the overlappingprotection into account and share the same thread as the regular execution.

@jamesmh
Copy link
Owner

jamesmh commented Jul 2, 2019

The "prevent overlap" feature only applies to jobs that are run within the scheduler.

Adding a ".Once()" method to the scheduler could work. But that means having to deploy the application whenever you want to run it? That doesn't sound like a great solution.

I'm thinking that introducing a new construct altogether would be better. Something like a new interface:

interface IPreventOverlap
{
     string PreventOverlapKey() { get; }
}

Implemented by an invocable, it could be used to simply tell that specific invocable that it will always prevent overlap (no need to specify when scheduling it either).

If queued, it would try to get a lock etc. (in the same way described below).

A potential fix (for now if it's critical...) is to manually try to lock the job using the same Mutex that the scheduler uses. You would inject IMutex from the DI container and call bool TryGetLock(string key, int timeoutMinutes) and void Release(string key)

// Wherever you are manually launching the job from...

IMutex mutex = this._mutex; // From DI
int timeoutInMinutes = 60; // This will release the key after 60 min in the event something bad happens.
string overlapKey = "MyPreventOverlapKey"; // Same key used in ".PreventOverlap()"
bool available = mutex.TryGetLock(overlapKey, timeoutInMinutes);

if(available)
{
    try 
    {
        await new MyInvocable().Invoke(); // run the job
    }
    finally
    {
        mutex.Release(overlapKey);
    }
}

For reference, in the scheduler, this logic is here

That being said, relying on the internals of Coravel is not a best practice lol.

I like the interface options as a potential new feature 😅

@oltronix
Copy link
Author

oltronix commented Jul 2, 2019

Adding the overlap protection as an interface sounds like a good way of solving it! I'm not in a panic, was looking at building a PoC for replacing our current background workers and this looks like a pretty good fit so far👍

About the Once() suggestion, you can change the schedule at runtime tough? I just tested injecting a IScheduler into a api controller and adding to the schedule in one of its actions. Seemed to work pretty well, except it leaves a recurring thing where I just wanted to trigger it once. :)

So if I could add it as scheduled once and it would just be removed from the schedule after being executed that should work pretty well. And as a bonus there could be an overload taking a DateTime, for scheduling a one off job in the future.

Thanks for the quick feedback!

@jamesmh
Copy link
Owner

jamesmh commented Jul 2, 2019

Nice! I like that idea. Similar some of the stuff hangfire offers.

Now that you lay out that scenario, I like the idea of a Once() method...

The only issues I can think of with being able to schedule something in the future is that, right now, schedules are not persisted. So, if you were to schedule something for, let's say, an hour from now. If your app restarts / is redeployed, that scheduled job will disappear.

I suppose for short term jobs that OK... Any thoughts?

@oltronix
Copy link
Author

oltronix commented Jul 2, 2019

Yeah, should keep that as a disclaimer, I saw persistence was part of the plus project, so might not be on the road map for this version. But a motivated user could still persist these DateTimes somewhere and restore them when the application starts again. And for us the running it once on demand, without having to worry about the same thing running in parallel, is the biggest thing anyway. This just seems like a good extension point for scheduling one off jobs in the future.

@jamesmh
Copy link
Owner

jamesmh commented Feb 28, 2020

To recap, there are two concerns here:

  • prevent overlapping between scheduler and queue for a single invocable
  • add a Once method to the scheduler

Ex:

scheduler.EveryHour().Once();

Related, a way to schedule once but using a duration:

scheduler.In(DateTime.FromMinutes(5))

@jamesmh
Copy link
Owner

jamesmh commented Jun 2, 2020

@oltronix it's been a while on this issue... Curious if this is still needed?

If so, wondering if using the queue instead would work. Something like QueueIn(TimeSpan time);?

Or were you thinking that you need an exact time in the future -> like May 19th, 2020?

@oltronix
Copy link
Author

oltronix commented Jun 3, 2020

Sorry for my inactivity, the prototype I was working on was put on ice and I forgot to respond here.

What I ended up doing was to inject the mutex as you suggested and it seemed to work fine.

Just to answer you questions:
For us the main concern was the need to be able to manually trigger a job that respected the restriction of only running one instance of the invocable at a time. Since the queue and the scheduler are separate I felt going with the scheduler was an good idea since that would give the 'schedule one run in the future'-feature more or less for free.

@jamesmh
Copy link
Owner

jamesmh commented Jun 3, 2020

Great, thanks for the feedback 👍

@renevdhoek
Copy link

Hi James, I am also looking for a solution to run an invocable on demand while respecting PreventOverlapping.
Can you give some insight on how it is implemented in Coravel Pro? And maybe you have new information on the 'schedule one run in the future'-feature

@jamesmh
Copy link
Owner

jamesmh commented Sep 29, 2021

Hey,

Right now you would have to use the Mutex explicitly as mentioned above. I've got another feature on the go that's similar to this one so after that one this might be a good next step to address the two issues around (a) running a one-off invocable and (b) making sure that one off can prevent overlap too.

I'll ping this thread once progress is made 👍

@nicoletacristian
Copy link

I also need this feature. Meanwhile, I'm going to implement the Mutex version.

@mnj
Copy link

mnj commented May 4, 2023

Same here, it would be nice, if we could schedule a "one of" run of a scheduled task, let's say something runs daily, and we need to rerun something during the day in special cases, it would be nice to be able to just trigger the task, and prevent it from overlapping.

@jamesmh
Copy link
Owner

jamesmh commented Aug 18, 2023

Implemented Once() functionality. See docs.

This contained a refactoring that will help with setting up queue and scheduler linked overlap prevention.

@jamesmh jamesmh self-assigned this Aug 18, 2023
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

No branches or pull requests

5 participants