diff --git a/internal/manager/api_impl/jobs_query.go b/internal/manager/api_impl/jobs_query.go index 74887d5f..f2218055 100644 --- a/internal/manager/api_impl/jobs_query.go +++ b/internal/manager/api_impl/jobs_query.go @@ -18,10 +18,8 @@ import ( // to the HTTP client if it cannot. Returns `nil` in the latter case, and the // error returned can then be returned from the Echo handler function. func (f *Flamenco) fetchJob(e echo.Context, logger zerolog.Logger, jobID string) (*persistence.Job, error) { - // TODO: use a timeout for fetching jobs. - // ctx, cancel := context.WithTimeout(e.Request().Context(), fetchJobTimeout) - // defer cancel() - ctx := e.Request().Context() + ctx, cancel := context.WithTimeout(e.Request().Context(), fetchJobTimeout) + defer cancel() if !uuid.IsValid(jobID) { logger.Debug().Msg("invalid job ID received") diff --git a/internal/manager/api_impl/jobs_test.go b/internal/manager/api_impl/jobs_test.go index 5df46825..a6d5b4d4 100644 --- a/internal/manager/api_impl/jobs_test.go +++ b/internal/manager/api_impl/jobs_test.go @@ -14,6 +14,7 @@ import ( "git.blender.org/flamenco/internal/manager/last_rendered" "git.blender.org/flamenco/internal/manager/persistence" "git.blender.org/flamenco/pkg/api" + "git.blender.org/flamenco/pkg/moremock" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" ) @@ -408,7 +409,9 @@ func TestSetJobPrio_nonexistentJob(t *testing.T) { jobID := "18a9b096-d77e-438c-9be2-74397038298b" prioUpdate := api.JobPriorityChange{Priority: 47} - mf.persistence.EXPECT().FetchJob(gomock.Any(), jobID).Return(nil, persistence.ErrJobNotFound) + mf.persistence.EXPECT(). + FetchJob(moremock.ContextWithDeadline(), jobID). + Return(nil, persistence.ErrJobNotFound) // Do the call. echoCtx := mf.prepareMockedJSONRequest(prioUpdate) @@ -438,7 +441,7 @@ func TestSetJobPrio(t *testing.T) { // Set up expectations. ctx := echoCtx.Request().Context() - mf.persistence.EXPECT().FetchJob(ctx, jobID).Return(&dbJob, nil).AnyTimes() + mf.persistence.EXPECT().FetchJob(moremock.ContextWithDeadline(), jobID).Return(&dbJob, nil).AnyTimes() jobWithNewPrio := dbJob jobWithNewPrio.Priority = 47 mf.persistence.EXPECT().SaveJobPriority(gomock.Not(ctx), &jobWithNewPrio) @@ -482,7 +485,7 @@ func TestSetJobStatusFailedToRequeueing(t *testing.T) { // Set up expectations. echoCtx := mf.prepareMockedJSONRequest(statusUpdate) ctx := echoCtx.Request().Context() - mf.persistence.EXPECT().FetchJob(ctx, jobID).Return(&dbJob, nil) + mf.persistence.EXPECT().FetchJob(moremock.ContextWithDeadline(), jobID).Return(&dbJob, nil) mf.stateMachine.EXPECT().JobStatusChange(ctx, &dbJob, statusUpdate.Status, "someone pushed a button") mf.persistence.EXPECT().ClearFailureListOfJob(ctx, &dbJob) mf.persistence.EXPECT().ClearJobBlocklist(ctx, &dbJob) diff --git a/internal/manager/api_impl/timeouts.go b/internal/manager/api_impl/timeouts.go new file mode 100644 index 00000000..756e5ce6 --- /dev/null +++ b/internal/manager/api_impl/timeouts.go @@ -0,0 +1,9 @@ +package api_impl + +// SPDX-License-Identifier: GPL-3.0-or-later + +import "time" + +const ( + fetchJobTimeout = 5 * time.Second +) diff --git a/pkg/moremock/context.go b/pkg/moremock/context.go new file mode 100644 index 00000000..c557c873 --- /dev/null +++ b/pkg/moremock/context.go @@ -0,0 +1,36 @@ +package moremock + +// SPDX-License-Identifier: GPL-3.0-or-later + +import ( + "context" + "time" + + "github.com/golang/mock/gomock" +) + +// ContextWithDeadline returns a gomock matcher to match a context.Context() +// with a deadline in the future. +func ContextWithDeadline() gomock.Matcher { return ctxWithDeadlineMatcher{} } + +type ctxWithDeadlineMatcher struct{} + +// Matches returns whether x is a match. +func (m ctxWithDeadlineMatcher) Matches(x interface{}) bool { + ctx, ok := x.(context.Context) + if !ok { + return false + } + + deadline, ok := ctx.Deadline() + if !ok { + return false + } + + return time.Now().Before(deadline) +} + +// String describes what the matcher matches. +func (m ctxWithDeadlineMatcher) String() string { + return "is context with deadline in the future" +} diff --git a/pkg/moremock/context_test.go b/pkg/moremock/context_test.go new file mode 100644 index 00000000..8a563fdb --- /dev/null +++ b/pkg/moremock/context_test.go @@ -0,0 +1,46 @@ +package moremock + +// SPDX-License-Identifier: GPL-3.0-or-later + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestCtxMatcher(t *testing.T) { + m := ctxWithDeadlineMatcher{} + + // Non-context types -> No match. + assert.False(t, m.Matches(nil)) + assert.False(t, m.Matches("something else")) + var nilValuedInterface context.Context + assert.False(t, m.Matches(nilValuedInterface)) + + // Context without deadlines -> No match. + assert.False(t, m.Matches(context.Background())) + assert.False(t, m.Matches(context.TODO())) + + // Deadline in the past -> No match. + { + past, cancel := context.WithDeadline(context.Background(), time.Now().Add(-1*time.Second)) + defer cancel() + assert.False(t, m.Matches(past)) + } + + // Deadline in the future -> Match. + { + future, cancel := context.WithDeadline(context.Background(), time.Now().Add(time.Second)) + defer cancel() + assert.True(t, m.Matches(future)) + } + + // Timeout in the future -> Match. + { + future, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + assert.True(t, m.Matches(future)) + } +} diff --git a/pkg/moremock/moremock.go b/pkg/moremock/moremock.go new file mode 100644 index 00000000..207b407b --- /dev/null +++ b/pkg/moremock/moremock.go @@ -0,0 +1,4 @@ +// Package moremock contains extra matchers for the gomock package. +// +// For info on gomock, see https://pkg.go.dev/github.com/golang/mock/gomock +package moremock