WangYihang / gojob

Go(od) Job is a simple job scheduler that supports task retries, logging, and task sharding.
MIT License
10 stars 1 forks source link

add support for Task with context as the function argument #6

Open emar-kar opened 5 months ago

emar-kar commented 5 months ago

In current realisation in case if any of the tasks is a long running function, (s *Scheduler) Wait() might hang for a long period of time, before exiting. If the job function hangs, it will consume the resources, staying as a running goroutine forever. It will not hang the worker, since it will return on timeout, but it will still be in scheduler and consume resources.

I suggest to add TaskWithContext instead of the regular Task and make it a standard. It will allow to add such parameters for scheduler as the real timeout. Give caller more flexible controls of the task execution and will allow to actually gracefully stop the task.

WangYihang commented 5 months ago

@emar-kar Thanks for your contribution! I will check it out soon.

WangYihang commented 5 months ago

You are right, when f hangs and the timeout signal arrived, the resource in f won't be release. We need a method to pass the cancel signal to the task object.

Maybe we should change the signature of func Do() error in the Task interface?

// Task is an interface that defines a task
type Task interface {
    // Do starts the task, returns error if failed
    // If an error is returned, the task will be retried until MaxRetries
    // You can set MaxRetries by calling SetMaxRetries on the scheduler
    Do(ctx context.Context) error
}

And the RunWithTimeout may becomes like this.

func RunWithTimeout(f func(ctx context.Context) error, timeout time.Duration) error {
    ctx, cancel := context.WithTimeout(context.Background(), timeout)
    defer cancel()

    done := make(chan error, 1)

    go func() {
        done <- f(ctx)
    }()

    select {
    case <-ctx.Done():
        return ctx.Err()
    case err := <-done:
        return err
    }
}

Or we might need to add additional context for RunWithTimeout? Or just embed the context into the concrete Task struct?

What's your option on this change?

emar-kar commented 5 months ago

Hello there, I'm currently looking into the code, to see what I can suggest here. I'll introduce my ideas for you a little bit later.