Refactoring background jobs&worker integration

[realLiangshiwei]

Resolved: #3365.
Resolved: #3422

• Implement disabled for background job & worker providers.
• Make method virtual.
• Allow developers decide whether to automatically register workers.
• Background job quartz integration exception retry and customize.

[realLiangshiwei]

In addition, the quartz background worker provider did not implement IsEnabled, I will sent another PR to solve this problem

[realLiangshiwei]

In order to be compatible with background job documents, I determine the IsJobExecutionEnabled property when executing the job

[realLiangshiwei]

@olicooper Could you check if it has been resolved #3422 ? Thanks.

[olicooper]

@liangshiw In your tests (see #3427 (comment)) you used StoreDurablility(true) which is something I didn't have configured for y jobs. I imagine this would have to be added to every JobDetail that is created. I wonder if we can use the other AddJob overload so it doesn't throw the exception "Jobs added with no trigger must be durable" like this one does. I haven't tried it out myself yet.

https://github.com/quartznet/quartznet/blob/d4241f02e8519cab8ea6af1fc359bd31357d383b/src/Quartz/Core/QuartzScheduler.cs#L685-L701

[realLiangshiwei]

@olicooper
For jobs stored in memory, this is no problem.
I can provide an option to automatically add StoreDurablility(true) to JobDetail.

[olicooper]

I have noticed that Quartz takes the timezone from the host system rather than from IClock. It means that Quartz thinks that jobs have misfired when they haven't. You can see the timezone data in QRTZ_CRON_TRIGGERS table:

Is this something you'd be able to implement too?

EDIT: I believe this could be an issue with my implementation rather than an issue with Quartz. I will test this more and let you know if anything needs fixing.

[olicooper]

Sorry for this.. After more testing I have found an edge case..
I have a job that generates statistics for each tenant at 4am every day for the previous day. If this job is missed I need ALL missed jobs to fire. Using WithMisfireHandlingInstructionIgnoreMisfires works well but calling RescheduleJob straight after ResumeJob means only 1 instance is executed before the trigger is deleted and restarted. If I comment out RescheduleJob in QuartzBackgroundWorkerManager it works as expected (but won't update the trigger!). This is proving to be a complicated thing to implement!

abp/framework/src/Volo.Abp.BackgroundWorkers.Quartz/Volo/Abp/BackgroundWorkers/Quartz/QuartzBackgroundWorkerManager.cs

Line 48 in ae72356

await _scheduler.RescheduleJob(quartzWork.Trigger.Key, quartzWork.Trigger);

What would be great is a method similar to 'AddJob' called 'AddTrigger' where we can replace the trigger but keep the old fire times until all the misfires have been fired. Quartz fires events off when things happen in the system, and I wonder if we can utilise these to call 'reschedule' at the appropriate time?

Also, thank you for adding 'virtual' to the methods, this has helped a lot with testing! :)

This is my job trigger:

Trigger = TriggerBuilder.Create()
    .WithIdentity($"{nameof(TenantStatisticsGenerationJob)}")
    // If the schedule is missed, fire ALL missed jobs up to the current time
    .WithCronSchedule("0 0 4 * * ?", opt => opt.WithMisfireHandlingInstructionIgnoreMisfires())
    .StartNow().Build();

UPDATE: To clarify the issue...

If we add this - Job misfires are handled properly, but changes made to the IQuartzBackgroundWorker.Trigger are not persisted to the database.

await _scheduler.AddJob(quartzWork.JobDetail, true);
await _scheduler.ResumeJob(quartzWork.JobDetail.Key);

If we add this - Job misfires are not handled properly (only 1 misfire handled) but changes made to the IQuartzBackgroundWorker.Trigger are persisted to the database.

await _scheduler.AddJob(quartzWork.JobDetail, true);
await _scheduler.ResumeJob(quartzWork.JobDetail.Key);
await _scheduler.RescheduleJob(quartzWork.Trigger.Key, quartzWork.Trigger);

If we add this - Job misfires are not handled at all, but changes made to the IQuartzBackgroundWorker.Trigger are persisted to the database.

await _scheduler.AddJob(quartzWork.JobDetail, true);
await _scheduler.RescheduleJob(quartzWork.Trigger.Key, quartzWork.Trigger);

[realLiangshiwei]

Hi @olicooper
This is a complicated matter and should be decided by the developer, so I added the AutoRegister property to skip the automatic registration.

[olicooper]

Yeah I figured this wouldn't be simple to fix. I will see if I can find anything that might help. Thank you for your work so far liangshiw.

@hikalkan The current version of this code improves the existing implementation and fixes some bugs so it is probably worth merging in for the next version and maybe we can open a ticket to resolve the remaining issues?

[realLiangshiwei]

@olicooper
I think this is too complicated, the developer should be responsible for job, and the framework should provide extensions where appropriate. Please see 9fac2ba.

[olicooper]

@liangshiw I have tried to simplify and improve the code I wrote above. The problem I am trying to solve is the issue with misfire instructions not being properly handled (i.e. WithMisfireHandlingInstructionIgnoreMisfires() - see #3382 (comment)). I have found another way to make the RescheduleJob update the trigger, but also preserve the original fire times so that the scheduler resumes the jobs properly. My previous code didn't really handle it well but this fixes all the issues in one. This code should allow the Quartz scheduler to work as intended for any Trigger modification. The code still looks a little bit complicated but it is really simple and because it fixes an existing bug, I believe it is required for the Quartz implementation to work properly with ABP.

This time I check if any of the schedule related properties have changed for the ITrigger. If the properties have not changed then I copy the previous and next fire times etc. over to the new trigger before calling RescheduleJob. I carefully checked the Quartz source code to see how RescheduleJob works and I found that this was a feasible solution. Ideally this kind of logic should be within the quartznet codebase, but it isn't and quartz hasn't been updated since 2018.

I have tested changing the type of trigger schedule, updating various Trigger details, causing misfires by leaving application not running for a while etc. and all scenarios seem to work as expected. More testing will be required before releasing it though. @maliming Does this seem like a feasible solution to fix the bug?

You can refactor it how you like but this is the basics:

protected virtual async Task DefaultScheduleJobAsync(IQuartzBackgroundWorker quartzWork)
{
    if (await _scheduler.CheckExists(quartzWork.JobDetail.Key))
    {
        await _scheduler.AddJob(quartzWork.JobDetail, true, true);
                
        var originalTrigger = await _scheduler.GetTrigger(quartzWork.Trigger.Key);
                
        if (ShouldPreserveTriggerFireState(originalTrigger, quartzWork.Trigger))
        {
            quartzWork.Trigger = CopyTriggerFireState(originalTrigger, quartzWork.Trigger);
        }

        await _scheduler.RescheduleJob(quartzWork.Trigger.Key, quartzWork.Trigger);
    }
    else
    {
        await _scheduler.ScheduleJob(quartzWork.JobDetail, quartzWork.Trigger);
    }
}

protected bool ShouldPreserveTriggerFireState(ITrigger oldTrigger, ITrigger newTrigger)
{
    // bypass object casting if the type doesn't match anyway
    if (oldTrigger.GetType() != newTrigger.GetType()) return false;
    //if (oldTrigger.HasMillisecondPrecision != newTrigger.HasMillisecondPrecision) return false;

    switch (oldTrigger)
    {
        // CRON TRIGGER
        case ICronTrigger oldCronTrigger when newTrigger is ICronTrigger newCronTrigger:
            if (oldCronTrigger.CronExpressionString != newCronTrigger.CronExpressionString) return false;
            if (!oldCronTrigger.TimeZone.Equals(newCronTrigger.TimeZone)) return false;
            break;
        // SIMPLE TRIGGER
        case ISimpleTrigger oldSimplTrigger when newTrigger is ISimpleTrigger newSimplTrigger:
            if (oldSimplTrigger.RepeatCount != newSimplTrigger.RepeatCount) return false;
            if (oldSimplTrigger.RepeatInterval != newSimplTrigger.RepeatInterval) return false;
            break;
        // DAILY INTERVAL TRIGGER
        case IDailyTimeIntervalTrigger oldDailyTrigger when newTrigger is IDailyTimeIntervalTrigger newDailyTrigger:
            if (oldDailyTrigger.RepeatCount != newDailyTrigger.RepeatCount) return false;
            if (oldDailyTrigger.RepeatIntervalUnit != newDailyTrigger.RepeatIntervalUnit) return false;
            if (oldDailyTrigger.RepeatInterval != newDailyTrigger.RepeatInterval) return false;
            if (((oldDailyTrigger.StartTimeOfDay == null) != (newDailyTrigger.StartTimeOfDay == null)) || !oldDailyTrigger.StartTimeOfDay.Equals(newDailyTrigger.StartTimeOfDay)) return false;
            if (((oldDailyTrigger.EndTimeOfDay == null) != (newDailyTrigger.EndTimeOfDay == null)) || !oldDailyTrigger.EndTimeOfDay.Equals(newDailyTrigger.EndTimeOfDay)) return false;
            if (((oldDailyTrigger.DaysOfWeek == null) != (newDailyTrigger.DaysOfWeek == null)) || oldDailyTrigger.DaysOfWeek.Count != newDailyTrigger.DaysOfWeek.Count) return false;
            if (!oldDailyTrigger.TimeZone.Equals(newDailyTrigger.TimeZone)) return false;
            break;
        // CALENDAR INTERVAL TRIGGER
        case ICalendarIntervalTrigger oldCalendarTrigger when newTrigger is ICalendarIntervalTrigger newCalendarTrigger:
            if (oldCalendarTrigger.RepeatIntervalUnit != newCalendarTrigger.RepeatIntervalUnit) return false;
            if (oldCalendarTrigger.RepeatInterval != newCalendarTrigger.RepeatInterval) return false;
            if (!oldCalendarTrigger.TimeZone.Equals(newCalendarTrigger.TimeZone)) return false;
            if (oldCalendarTrigger.PreserveHourOfDayAcrossDaylightSavings != newCalendarTrigger.PreserveHourOfDayAcrossDaylightSavings) return false;
            if (oldCalendarTrigger.SkipDayIfHourDoesNotExist != newCalendarTrigger.SkipDayIfHourDoesNotExist) return false;
            break;
        default:
            return false;
    }
            
    return true;
}

protected ITrigger CopyTriggerFireState(ITrigger fromTrigger, ITrigger toTrigger)
{
    var triggertoUpdate = toTrigger;

    if (triggertoUpdate is AbstractTrigger abstractTrigger)
    {
        abstractTrigger.SetPreviousFireTimeUtc(fromTrigger.GetPreviousFireTimeUtc());
        abstractTrigger.SetNextFireTimeUtc(fromTrigger.GetNextFireTimeUtc());
        abstractTrigger.StartTimeUtc = fromTrigger.StartTimeUtc;
    }

    switch (fromTrigger)
    {
        // CRON TRIGGER
        //case ICronTrigger fromCronTrigger when triggertoUpdate is ICronTrigger toCronTrigger:
        //    break;
        // SIMPLE TRIGGER
        case ISimpleTrigger fromSimplTrigger when triggertoUpdate is ISimpleTrigger toSimplTrigger:
            toSimplTrigger.TimesTriggered = fromSimplTrigger.TimesTriggered;
            break;
        // DAILY INTERVAL TRIGGER
        case IDailyTimeIntervalTrigger fromDailyTrigger when triggertoUpdate is IDailyTimeIntervalTrigger toDailyTrigger:
            toDailyTrigger.TimesTriggered = fromDailyTrigger.TimesTriggered;
            break;
        // CALENDAR INTERVAL TRIGGER
        case ICalendarIntervalTrigger fromCalendarTrigger when triggertoUpdate is ICalendarIntervalTrigger toCalendarTrigger:
            toCalendarTrigger.TimesTriggered = fromCalendarTrigger.TimesTriggered;
            break;
    }

    return triggertoUpdate;
}

[realLiangshiwei]

@olicooper
I think allowed to disable automatic registration and provide extensions, this is enough.

[olicooper]

Hi @liangshiw would it be possible to create a separate PR to make the methods virtual in the QuartzBackgroundWorkerManager so that it can be merged for the next (v2.7) release? @hikalkan moved this PR to v2.8 which isn't going to be released until 21st May.

[realLiangshiwei]

Hi @olicooper, sorry for the late reply.
Currently you can replace the implementation class.

[hikalkan]

@liangshiw I suppose this work has been finalized, so I will review and merge?

[realLiangshiwei]

@hikalkan Yes, it's readly, please review.

[hikalkan]

Thank you for the implementation. I tested with hangfire and rabbitmq, it is working as expected:

• Used demos on the https://github.com/abpframework/abp/tree/dev/modules/background-jobs/app to test it
• I disabled job execution, created some jobs and I see they are added to queue but not executed.
• I stopped the application, enabled job execution, re-started and I see previous jobs also has executed. So, no job loss.

For Quartz, jobs are lost. Probably because I haven't used a persistence system for quartz, I don't have much experience on it.

BTW, I fixed a bug: 783c162