Fixing Enqueued Job Trigger for Multiple Queues

[Tinyakov]

There was a significant issue with the new enqueued job trigger that should release waiting in jobs fetching loops. It used only one static AutoResetEvent instance for all workers and queues. When this AutoResetEvent is released, it only releases one random waiting thread. If it was a thread of a worker with a different queue, the job gets stuck until a new event or the QueuePollInterval passes. This situation frequently occurred in my environment.

• Introduced new class AutoResetEventRegistry in place of one AutoResetEvent. It internally holds several AutoResetEvent instances, one for each queue.
• Moved its triggering from PostgreSqlStorage to PostgreSqlWriteOnlyTransaction, where new jobs are added and committed. The list _queuesWithAddedJobs accumulates all queues with added jobs in the transaction, and only those queues fire events after commit. This approach also reduces false positive triggers, as not every transaction contains new jobs.
• Minor: Removed cancellationToken.ThrowIfCancellationRequested() after waiting in the fetch loop since it is already executed at the beginning of each iteration.

Concerns:

• I didn't make any changes to the Dequeue_UpdateCount method, which is used in case of UseNativeDatabaseTransactions = false, because there was originally no signal waiting. I'm not sure if it should be modified.

[dmitry-vychikov]

Ideally, this could use postgres notifications channel. https://www.postgresql.org/docs/current/sql-notify.html
Then it will work even for multi process / multi server scenario.

I think there were usages of such approach already in the code.

[Tinyakov]

@dmitry-vychikov , Postgres LISTEN/NOTIFY function requires a long-living session/connection which comes at a cost. Hangfire.Postgres provides an option for using it, but I prefer not to enable it.

I believe there are two quite different use cases:

1. Single-process, where fetching signals work accurately and efficiently now.
2. Multi-process, where I agree that current LISTEN/NOTIFY implementation could be improved by separating it into queues that can be included in notification payload.

This PR is improving single-process scenario.

[dmitry-vychikov]

@Tinyakov

long-living session/connection which comes at a cost. Hangfire.Postgres provides an option for using it, but I prefer not to enable it.

Do you have any benchmarks or experience to share? How high is that performance cost? Just interested.

I'm not totally against this improvement. But to me using postgres channels looks simpler because it can cover both cases. It will be less code in total to maintain which is a good thing in general. Now it is a bit messy because of combination of multiple approaches in different places.

[Tinyakov]

@dmitry-vychikov ,

Do you have any benchmarks or experience to share? How high is that performance cost? Just interested.

I can't even turn EnableLongPolling on, cause all of my environments have PgBouncer in transaction mode in front of Postgres.

In terms of performance and costs, I can agree that one long polling connection would be ok. However, according the current code there are one connection per worker. It can be improved also, but as I mentioned above, long-living connections don't fit everyone.

BTW, I agree that current code quite branching. I tried not to make it more complicated.

[Tinyakov]

Hi @azygis ! Have you had a chance to look at this PR? I noticed that you fixed the test pipeline. What do you think about? Should I invite someone else?

[azygis]

I did leave a few remarks which don't seem to have been addressed yet.

[Tinyakov]

@azygis , sorry, I can't find your remarks. All conversations are resolved now.
What remarks are you talking about?

[Tinyakov]

@Tinyakov Tinyakov requested a review from dmitry-vychikov

Sorry, it was a click by mistake.

[azygis]

Shoot, sorry, I always forget that GitHub requires submitting the review as opposed to Azure DevOps where comments are just appearing automatically. Published it now.

[azygis]

The package has been published.