Getting available jobs uses an orphan context which loses important information passing

[rgalanakis]

There are a few places in the River code which use context.Background() to bypass potential cancellation, like this:

func (p *producer) dispatchWork(count int, fetchResultCh chan<- producerFetchResult) {
	// This intentionally uses a background context because we don't want it to
	// get cancelled if the producer is asked to shut down. In that situation, we
	// want to finish fetching any jobs we are in the midst of fetching, work
	// them, and then stop. Otherwise we'd have a risk of shutting down when we
	// had already fetched jobs in the database, leaving those jobs stranded. We'd
	// then potentially have to release them back to the queue.
	jobs, err := p.exec.JobGetAvailable(context.Background(), &riverdriver.JobGetAvailableParams{

and here:

func (s *Subscription) Unlisten(ctx context.Context) {
	s.unlistenOnce.Do(func() {
		// Unlisten uses background context in case of cancellation.
		if err := s.notifier.unlisten(context.Background(), s); err != nil { //nolint:contextcheck
			s.notifier.Logger.ErrorContext(ctx, s.notifier.Name+": Error unlistening on topic", "err", err, "topic", s.topic)
		}
	})
}

I recently adding a pgx.QueryTracer to my pgx pool that logs SQL statements.

I don't want to log River SQL statements, since we're already using River's built-in logging capabilities.

To solve this, I can call River with code like this:

riverClient.Start(context.WithValue(ctx, LoggingTracerIgnore, true))

And then in my tracer, if LoggingTracerIgnore is in the context, I skip logging.

However, in places where River makes a context.Background, I can't tell the context that "hey, this code is from River, so don't log it."

Perhaps as a configuration option, it would be useful to have a 'context factory' that defaults to context.Background, so callers can add values to the context? Obviously it can be abused, but I think it's a low-risk exposure. Alternatively, I can see 1) identifying River as the caller using a context key or 2) a callback run on all contexts, as potentially working, and also eliminating some of the wrapping I'm having to do to put in the 'ignore logging' context key. There's also 3) using a different pool, but that eliminates the possibility for things like transactional jobs, and also increases connection counts.

Other than that, thank you for this library! It's extremely well designed.

[bgentry]

I think we can instead tweak this to use context.WithoutCancel. It was brand new when we wrote the initial job fetching code and wouldn’t have been supported by the two latest Go releases (our support target) but that’s no longer an issue.

There are very few circumstances where we specifically want to avoid inheriting a deadline or cancellation due to safety. But we have no reason to care about context values being propagated (other than ones we specifically add) so this should be perfectly fine.

[bgentry]

Should be resolved by #514 in the next release, thanks for the report!