2

I have a set of jobs that I need executed. The entity that manages these jobs is responsible for composing the jobs and starting them. When a job is finished, the job uses a callback function to inform the entity of the result of the job (Whether it threw an exception and such)

public class TaskSet {
    Queue<Job> JobsToExecute;
    void executeJob(Job j);
    void jobFinishedCallback(Job j);
}

public class Job {
    Action<Job> jobFinishedCallback();
    public void Execute() { jobFinishedCallback(this); }
}

When a job has failed, the jobFinishedCallback calls the TaskSet's jobFinishedCallback. If the job failed, jobFinishedCallback in TaskSet will call executeJob again. If a job fails 100 times, the call stack grows accordingly. Execute -> jobFInishedCallback -> executeJob ->and so on.

I added a maximum retry count, but I still feel uncomfortable having large stacks if a job keeps failing, I believe it is a code smell.

Does anyone have a good suggestion to avoid this problem?

  • 1
    Your Execute() simply calls jobFinishedCallback(this). That should be handled outside your job. In other words, you can have a continuation to handle the decision to requeue, which also serves to break the cycle. – Berin Loritsch May 1 '18 at 12:56
5

I have a set of jobs that I need executed.

Excellent. You have chosen a reasonable architecture to meet this need.

When a job is finished, the job uses a callback function to inform the entity of the result of the job (Whether it threw an exception and such)

Congratulations, you have re-invented Task<T> and renamed it Job. Doing the same thing as an established library is evidence that you are on the right track, but it is also evidence that you may be repeating work that is already done.

Maybe just use Task<T>, if it meets your needs. Note that a task has a "continue with" feature where you can give the callback directly to the task, just as your Job does.

Moreover: C# allows you to create task-based asynchronous workflows very easily by using await. Callback-based asynchrony makes you turn your program logic inside out, and makes exception handling difficult. Let the compiler do the heavy lifting. It will rewrite your workflows into continuation passing style for you, and get the exception handling correct.

If the job failed, jobFinishedCallback in TaskSet will call executeJob again.

That's the wrong thing to do, as you have discovered.

Does anyone have a good suggestion to avoid this problem?

Don't call executeJob again. Enqueue the job back onto the work queue again, and return. That way not only do you not get an infinite regression, you also treat the retry fairly. The retry should have to wait its turn; all the jobs in the queue now should have priority over it.

Similarly, when a job finishes normally, don't call its continuation directly; you can run into the same problem. Enqueue a new job which calls the continuation, and give that job a null continuation. And again, this is more fair; the continuation should have to wait its turn, just like every other job in the queue is waiting its turn.

More generally: today would be a good day to do some research on tasks/futures/promises, reactive programming, observable sequences, trampolines, continuation passing style, actor model, and so on. You are re-inventing from scratch technologies that experts have been studying for decades, so learn from their accumulated knowledge, rather than your own trial and error. And use the tested, debugged, well-designed classes in the library, rather than rolling your own -- provided that they meet your needs. If they don't, please give feedback to Microsoft about why they do not.

C# tasks use the concept of "task context" to determine how continuations are scheduled; your re-invention is basically the context rules for GUI applications. The Windows message queue is used as the work queue. There are other choices that are possible; for example, some contexts will instead grab a worker thread off the pool and execute the continuation on the thread. By using the off-the-shelf parts already created you can make use of the flexibility and power already designed into the system.

| improve this answer | |
  • This is essentially the approach I was going to write up, but Eric beat me to it. Using Tasks and re-queuing the job is the right approach. – Berin Loritsch May 1 '18 at 17:24
  • JimmyJames, please stop editing my post. There is no snarkiness intended. When I congratulate someone, I am being sincere. When I tell someone that their architecture choices are excellent, I am being sincere. When I say to maybe do something, I mean evaluate the alternative and maybe do it if it meets your needs. Read more charitably. – Eric Lippert May 1 '18 at 17:34
  • 1
    Changing "maybe" to "you should" for example changes the meaning of the sentence. I am not making a positive recommendation to use tasks; I'm making the strong suggestion that the research should be done and a choice should be made working from a position of knowledge rather than ignorance. – Eric Lippert May 1 '18 at 17:41
  • 3
    @JimmyJames: I'll take that feedback in the constructive spirit in which it was intended. There's a pervasive bias in text-only communication which makes statements of fact sound hostile and well-meaning praise sound like condescension in the minds of many. I'll try to make my intent more clear. – Eric Lippert May 1 '18 at 19:38
  • 3
    @EricLippert: Edits like the ones Jimmy James proffered are almost certainly some form of response to this (somewhat unfortunate) blog post, and its resulting fallout on meta.stackoverflow.com – Robert Harvey May 1 '18 at 19:58
1

Don't have a callback. or at least dont pass the job into it.

TaskSet.executeJob(Job j)
{
    j.Execute();    
    //the job has finished
    if(j.status == failed)
    {
        this.Queue.Add(j); //queue for retry
    }
}
| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.