How do I prevent stack overflows from happening in this command structure with callbacks?

Question

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?

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 01 '18 at 12:56

Eric Lippert · Accepted Answer · 2018-05-01T19:37:19.257

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.

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 01 '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 01 '18 at 17:34
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 01 '18 at 17:41
Hey Eric, I thoroughly enjoyed your answer, but I think your assumption of me wanting to do the same thing as `Task` is the fault of my oversimplified example. A job is a whole lot more than what I portrayed it as. This aside, your answer is still sound. Thank you! — Zimano, May 01 '18 at 17:42
@Zimano: You're welcome. And thanks for nicely illustrating my point in the comment above! — Eric Lippert, May 01 '18 at 17:43
@EricLippert It comes off sarcastic to me but no offense was meant (read more charitably?) I was simply thinking about [this](https://stackoverflow.blog/2018/04/26/stack-overflow-isnt-very-welcoming-its-time-for-that-to-change/?cb=1). I described it as a suggestion and you've rejected it. There is no issue. I only edited it once. — JimmyJames, May 01 '18 at 19:32
@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 01 '18 at 19:38
@EricLippert And I will take your advice and read more charitably. It could just be me. Anyway, we agree that written text misses context queues. — JimmyJames, May 01 '18 at 19:45
@EricLippert: Edits like the ones Jimmy James proffered are almost certainly some form of response to [this (somewhat unfortunate) blog post](https://stackoverflow.blog/2018/04/26/stack-overflow-isnt-very-welcoming-its-time-for-that-to-change/), and its resulting fallout on https://meta.stackoverflow.com — Robert Harvey, May 01 '18 at 19:58
I think so too. It is very unfortunate that blog post had to be made. — Zimano, May 03 '18 at 07:26

score 1 · Answer 2 · answered May 01 '18 at 11:39

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
    }
}

answered May 01 '18 at 11:39

Ewan

70,664
5
76
161

How do I prevent stack overflows from happening in this command structure with callbacks?

2 Answers2