Disposing of SqlConnection before Disposing of SqlCommand

Question

I have a method like this:

void Execute(SqlCommand cmd)
{
    try
    {
        using(SqlConnection conn = new SqlConnection(...))
        {
            conn.Open();
            cmd.Connection = conn;
            // ...
        }
    }
    finally
    {
        cmd.Connection = null;
    }
}

It's used like this:

using(SqlCommand cmd = new SqlCommand(...))
{
    Execute(cmd);
}

Is there anything behaviorally wrong with this? I know it's more usual to create the SqlConnection first, then the SqlCommand, but since SqlCommand is in reality more-or-less a plain old object I think it should be fine in practice.

it would be strongly unexpected this pattern. why do tit this way? — Daniel A. White
– Daniel A. White, Commented Jan 6, 2023 at 16:18
This is strange because the owner of the command object lets others configure said command. This is usually not the way we go about it because this pattern is usually used when you want to reuse some logic using what you pass as parameters. If Execute's job were to configure the command, perhaps. However, it is a method uses the command, and this is why it looks awkward. Having said that, IDisposable.Dispose should work and not throw even if called multiple times. — José Ramírez
– José Ramírez, Commented Jan 6, 2023 at 16:28
Context is I'm working in a large legacy codebase with thousands of usages of the Execute method like written here. Except in the legacy implementation, one SqlConnection is shared with a lock used to control concurrent access. I'm trying to refactor to remove the lock and leverage built-in connection pooling in the least invasive way possible. — Stephen Swensen
– Stephen Swensen, Commented Jan 6, 2023 at 16:43
There is nothing wrong with that design pattern. In fact, it is the premise of how the CA.Blocks works [nuget.org/packages/CA.Blocks.SQLServerDataAccess]. You build a command and then hand the command off for execution. There are a couple of things to note. You need to do all the work with the database before the method is completed and secondly, you also need to set the CommandBehavior to CommandBehavior.CloseConnection. — Kevin Bosch
– Kevin Bosch, Commented Jan 6, 2023 at 20:21
I agree that there is nothing wrong with that approach (except it's a bit unusual, but that doesn't mean wrong). — Evk
– Evk, Commented Jan 9, 2023 at 7:45

Reza Kia · Accepted Answer · 2023-01-09 07:54:46Z

I highly recommend to rethink about refactoring to find better solution. But in your scenario

You can safety dispose connection before disposing command.

They are not related to each other and you can safety remove connection from a command object or even set it to another connection.

The only important thing is command must have an open connection before execute.

Removing connection before finally section make disposing of connection faster.

I recomend to rewrite your code to something like this:

void Execute(SqlCommand cmd)
{
    try
    {
        using (SqlConnection conn = new SqlConnection(""))
        {
            conn.Open();
            cmd.Connection = conn;

            // Execute command and get data

            // It is better to remove connection from command here
            cmd.Connection = null;
        }
    }
    catch
    {
       // Remove connection if there is error
        cmd.Connection = null;
    }
}

Collectives™ on Stack Overflow

Disposing of SqlConnection before Disposing of SqlCommand

1 Answer 1

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

Comments

Your Answer

Sign up or log in

Post as a guest

Related