[Bug] Connection already open on Database.Migrate()

[hexxone]

I just updated our codebase from ASP .NET6 to .NET8.

In the process of doing so, I was now also able to update the Npgsql.EntityFrameworkCore.PostgreSQL package from 7.0.11 to 8.0.4. I was since able to recreate the Migration with the new packages without problems.

However, since then was getting the following Error when debugging or running Unit Tests:

System.InvalidOperationException : Connection already open
TearDown : System.NullReferenceException : Object reference not set to an instance of an object.
StackTrace:    at Npgsql.ThrowHelper.ThrowInvalidOperationException(String message)
   at Npgsql.NpgsqlConnection.CheckClosed()
   at Npgsql.NpgsqlConnection.Open(Boolean async, CancellationToken cancellationToken)
   at Npgsql.NpgsqlConnection.Open()
   at Npgsql.EntityFrameworkCore.PostgreSQL.Migrations.Internal.NpgsqlMigrator.Migrate(String targetMigration)
   at Microsoft.EntityFrameworkCore.RelationalDatabaseFacadeExtensions.Migrate(DatabaseFacade databaseFacade)
   at Contexts.EntityDbContext.Init()

It seems the casue for the Issue is that I am manually opening a Transaction before migrating the Database.

This will in effect open the Connection, and afterwards Ef-postgres also tries to always open the Connection again.

Problematic Code:

    using var initMigration = context.BeginTransaction();
    context.Database.Migrate(); // <--- casues "Connection already open"
    initMigration.Commit();

This was however working in the previously installed Version (7.0.11) and so it's a regression/bug imo. Or was this an intended change?

For now the fix was to simply comment out the transaction-code:

    // using var initMigration = context.BeginTransaction();
    context.Database.Migrate(); // Ok
    // initMigration.Commit();

But I think the best solution would be to check whether the Connection is already Open, before trying to open it again.

Thanks in advance!

[hexxone]

Ok now there is a weird new behaviour:

I completely cleared all local caches from my computer. Now when running the Tests locally, everything works as expected with the additional Migration code commented in.

I am unsure as to why exactly that is, but when running in a Gitlab CI pipeline with the exact same settings, it still fails.

  Failed TestPlantControllerGet [758 ms]
  Error Message:
   System.InvalidOperationException : Connection already open
  Stack Trace:
     at Npgsql.ThrowHelper.ThrowInvalidOperationException(String message)
   at Npgsql.NpgsqlConnection.Open(Boolean async, CancellationToken cancellationToken)
   at Npgsql.NpgsqlConnection.Open()
   at Npgsql.EntityFrameworkCore.PostgreSQL.Migrations.Internal.NpgsqlMigrator.Migrate(String targetMigration)
   at Microsoft.EntityFrameworkCore.RelationalDatabaseFacadeExtensions.Migrate(DatabaseFacade databaseFacade)

I am just baffled at this point.

[azygis]

The same happens for us. We have a custom migration runner which has some extra stuff to do before running the migrations (EF/SQL scripts/data, in sequence). We are running the migrations one by one after getting them with GetPendingMigrationsAsync.

The baffling part on our side is that Connection already open is thrown AFTER the whole EF migration has been completed, even after provider adds the "version" to migrations table. I would assume there's nothing more to do after adding the migration name to the table, but it still tries to open the connection afterwards.

Restarting the migrations again after they fail responds no pending migrations so all is well. Our deployment makes sure to restart the application if it crashes so it's not the biggest deal, but still quite irritating.

Edit: I see. Checked the migrator, seems like the provider is trying to be a good guy and reload the types after enum migrations. I think it should check if the connection is closed before trying to open it.

[ShockSplash]

@roji, please pay attention to this problem!

[LarsWesselius]

Edit: I see. Checked the migrator, seems like the provider is trying to be a good guy and reload the types after enum migrations. I think it should check if the connection is closed before trying to open it.

And I guess it should only Close it if it was opened previously in the same method. Otherwise it should probably leave it open

I am also facing this issue and for now removing the transaction code.

[roji]

Note: EF 9.0 will automatically run migrations within a transaction, so there will be no need to manually start and commit transactions yourself around migrations.

In any case, can someone please put together minimal, runnable code sample (basically a quick console program) and post it here? That's always the right thing to do when posting an issue.

[LarsWesselius]

Sure, example posted here: https://github.com/LarsWesselius/npgsqlMigrationBug/tree/main

Can also confirm something like this fixes the issue but I can't pretend to have enough knowledge of the npgsql project to know whether this would cause issues or not elsewhere (NpgsqlMigrator.cs#126);

if (reloadTypes && _connection.DbConnection is NpgsqlConnection npgsqlConnection)
{
    bool shouldOpen = npgsqlConnection.State != System.Data.ConnectionState.Connecting && npgsqlConnection.State != System.Data.ConnectionState.Open;
    if (shouldOpen)
    {
        await npgsqlConnection.OpenAsync(cancellationToken).ConfigureAwait(false);
    }
    try
    {
        await npgsqlConnection.ReloadTypesAsync().ConfigureAwait(false);

        if (shouldOpen)
        {
            await npgsqlConnection.CloseAsync().ConfigureAwait(false);
        }
    }
    catch
    {
        await npgsqlConnection.CloseAsync().ConfigureAwait(false);
    }
}

[roji]

Thanks @LarsWesselius. I can see that there's probably a problem here... At some point I'll be doing a push on EFCore.PG work for 9.0, i'll try to address this as part of that.

[schmitch]

@roji isn't this a bug for 8.x? since actually in 8.x it's allowed to set a transaction in efcore?

Code from EFCore: https://github.com/dotnet/efcore/blob/ecfee78eb1fa2b2eaa0dbf945f1d4f8fa571be74/src/EFCore.Relational/Migrations/Internal/MigrationCommandExecutor.cs#L34C1-L34C61

Edit:

we basically manually reload the types, so we basically can just use the "original" Migrator

[azygis]

@roji

I see the milestone set to 9.0.0, and your last message implies it's only going to happen when it's time for 9.0.

Our CI build is taking more and more time because each enum migration terminates the further migrations (which we restart, but each restart takes some time and that adds up).

Can you confirm it's gonna be fixed only in 9.0+, which would be a shame? Or is there a plan to backport it to 8, considering it's LTS?

[roji]

@azygis I haven't yet looked into this issue - I definitely do intend to try to get it in for 9.0, but until I do, I can't really say whether it'll be backported or not. This generally isn't really a critical bug, as annoying as it may be.

[roji]

@LarsWesselius thanks again for the repro.

I've submitted #3343 to fix this and will backport to the next 8.0.x release.

Note that in EF 9.0, a new migration lock feature was added (see release notes) - this wraps the transaction in a lock, ensuring that concurrent instances of the application don't try to apply migrations at the same time. The lock requires a transaction, and so EF throws (this is regardless of the "connection already open" error being discussed here). So wrapping MigrateAsync() with your own user transaction is going to be problematic going forward in any case - but we can examine that separately.