Potential NpgsqlOperationInProgressException Exceptions

[b-rain-m]

I am evaluating this library for a project that is going to handle a lot of data.

I noticed that there is a chance of generating a Npgsql.NpgsqlOperationInProgressException exception that is likely to increase as you increase the frequency of queuing jobs within one application process.

To confirm this could happen I added a test to my fork of the library. Although the values in the options are not realistic, more realistic values will not eliminate the problem, just make it show up less.

    [Fact]
    [CleanDatabase]
    public void Queues_Can_Enqueue_Without_Race_Condition()
    {
      UseConnection((connection, storage) => {

        storage.Options.AllowUnsafeValues = true;
        storage.Options.QueuePollInterval = TimeSpan.FromMilliseconds(1); // Increase chances of race condition.

        PostgreSqlJobQueue queue = CreateJobQueue(storage, false);
        IFetchedJob job = null;

        var task = Task.Run(() => {
          CancellationTokenSource source = new CancellationTokenSource(TimeSpan.FromSeconds(60));

          while(!source.Token.IsCancellationRequested)
          {
            job = queue.Dequeue(new[] { "default" }, source.Token);
          }
        });
        // Give a head start to avoid race condition.
        Thread.Sleep(1000);
        Assert.Null(job);
        
        // Attempt to produce a race condition.
        while(!task.IsCompleted)
          queue.Enqueue(connection, "default", "1");

        Assert.True(true);

      });
    }  

I think the root problem is that overall the library attempts to use a single connection across multiple threads in a single application process. PostgreSQL connections do not support this use case so if one thread attempts to say queue a job while any of the other 20 or so threads are querying for jobs, one of the threads can generate a Npgsql.NpgsqlOperationInProgressException.

I'm not sure what will happen to the process overall if one of the worker threads throws this exception. However it seems like a bandaid would be to auto-retry if one of these exceptions is thrown.

A proper fix would be to create new connection objects for each call to Postgres. In theory the underlying Npgsql library should pool connections so recent connections can be left around for performance but hopefully not too many.

This pooling can be controlled: https://www.npgsql.org/doc/connection-string-parameters.html#pooling

Why was there an attempt to re-use the same connection throughout the application? Were there problems beforehand?

[b-rain-m]

Something else I'm noticing is in several places of the application there are explicit calls to open the connection but then right afterwards that connection I used by Dapper. Dapper implicitly opens connections that haven't been opened (and will close them afterwards if they were opened by Dapper).

See the linked code below.

https://github.com/b-rain-m/Hangfire.PostgreSql/blob/bd2cc537595889d6932a180010146ec7f3b550e7/src/Hangfire.PostgreSql/PostgreSqlDistributedLock.cs#L122-L144

[b-rain-m]

This is an update. After going over the source more thoroughly, it looks like this situation would only happen is you instantiate the Storage class with a connection passed in. This is done a lot in the unit tests which explains the race conditions I've been able to replicate.