Issues with injecting `river.Client` dependency

[hypnoglow]

Consider the following app structure (code is simplified):

// database layer component, depends on conn pool and river to insert jobs transactionally
store := postgres.NewStore(db, riverClient)

// ...

// Worker implementation that uses store
worker := some.NewWorker(store)

// ...

// River client which depends on workers
riverClient := river.NewClient(river.Config{Workers: workers})

As you see, there is a dependency injection loop here: store depends on river client to insert jobs, but river client depends on workers, and worker depends on store.

I believe this situation is not unique and should be quite common for many apps.

One solution we can use currently is set river client into store later, like this:

riverClient := river.NewClient(river.Config{Workers: workers})

store.SetRiverClient(riverClient)

But this is a step away from dependency injection principles, as constructor should be sufficient.

Another solution that comes to mind is to use two clients:

// This client is used to insert jobs
riverClient := river.NewClient(river.Config{Workers: nil})

store := postgres.NewStore(db, riverClient)

// This client manages workers
riverWorkersClient := river.NewClient(river.Config{Workers: workers})

riverWorkersClient.Start()

The drawback is we losing validation.

Maybe I'm not seeing a better solution to this, could you suggest?

I guess this problem is a consequence of the fact that river.Client is too thick. Maybe it would be reasonable to separate job producer from workers, so the first component is responsible for creating jobs, while the second one is all about managing work. This way we can properly solve dependency issues, in fact, similar projects (like asynq already designed the API this way.

[brandur]

Hi Igor,

A fairly straightforward workaround is to define your worker bundle separately, create the River client, then add workers after:

workers := river.NewWorkers()

riverClient := river.NewClient(river.Config{Workers: workers})

// Do your other stuff with `postgres.NewStore`

// Add workers here after client initialization
workers.Add(...)

// Start after adding all workers
if err := riverClient.Start(ctx); err != nil {
    ...
}

Another option is to not include the River client in postgres.NewStore, and instead, if you need to insert a job from a worker, access the client through context as described here [1]:

func (w *MyWorker) Work(ctx context.Context, job *river.Job[MyArgs]) error {
    client := river.ClientFromContext(ctx)

    ...
}

I guess this problem is a consequence of the fact that river.Client is too thick. Maybe it would be reasonable to separate job producer from workers, so the first component is responsible for creating jobs, while the second one is all about managing work. This way we can properly solve dependency issues, in fact, similar projects (like asynq already designed the API this way.

I guess a design like that is conceivable, but given the inertia on the current API and that there are some reasonably good/accessible workarounds, I doubt we'd go this route.

[1] https://riverqueue.com/docs/inserting-and-working-jobs

[hypnoglow]

@brandur thanks for prompt response.

A fairly straightforward workaround is to define your worker bundle separately, create the River client, then add workers after:

Oh, for me it wasn't obvious from the docs that this option is possible. Will try this, thanks.

if you need to insert a job from a worker, access the client through context

Most of my cases include inserting outside of a worker, so this is not an option.

I guess a design like that is conceivable, but given the inertia on the current API and that there are some reasonably good/accessible workarounds, I doubt we'd go this route.

This is quite understandable. Still, if you happen to reconsider API changes, maybe take another look into this problem to make library usage easier.

Thank you

[brandur]

Oh, for me it wasn't obvious from the docs that this option is possible. Will try this, thanks.

Excellent.

This is quite understandable. Still, if you happen to reconsider API changes, maybe take another look into this problem to make library usage easier.

Okay, we'll keep it mind.

In the meantime, going to close this out. Thanks!