9

How to fix this problem; connection already closed in my function:

SqlConnection con=new SqlConnection(@"Here is My Connection");

public void run_runcommand(string query)   
{   

    try   
    {   
        con.Open();   
        SqlCommand cmd1 = new SqlCommand(query, con);   

        cmd1.ExecuteNonQuery();    
        con.Close();    
    }    
    catch (Exception ex) { throw ex; }                        
}    
//...
try       
{           
    string query="my query";           
    db.run_runcommand(query);          
}         
catch(Exception ex)            
{         
    MessageBox.Show(ex.Message);              
}
Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
Naeem Shah
  • 115
  • 1
  • 1
  • 12

5 Answers5

29

I assume that the error is raised on this line:

con.Open(); // InvalidOperationException if it's already open

since you're reusing a connection and you probably have not closed it last time.

You should always close a connection immediately as soon as you're finished with it, best by using the using-statement:

public void run_runcommand(string query)   
{
    using(var con = new SqlConnection(connectionString))
    using(var cmd = new SqlCommand(query, con))
    {
        con.Open();
        // ...
    }  // close not needed since dispose also closes the connection
}

Note that you should not use a Catch block just to rethrow an exception. If you don't do anything with it don't catch it at all. It would be even better to use throw; instead of throw ex; to keep the stack trace. https://stackoverflow.com/a/4761295/284240

Community
  • 1
  • 1
Tim Schmelter
  • 450,073
  • 74
  • 686
  • 939
  • con.close() needed i will try both above both codes but still error will come when my query second time execute then error will be come other wise first time error will not shown – Naeem Shah Nov 12 '12 at 12:03
  • @NaeemShah: Then you don't have used `using` in your method to create the connection.So when you call the method a second time, the connection is created first and then opened. – Tim Schmelter Nov 12 '12 at 12:07
13

Better you write finally block and within it con.close() every where where you used try catch blocks. Eg.

public void run_runcommand(string query)   
{   
    try   
    {   
        con.Open();   
        SqlCommand cmd1 = new SqlCommand(query, con);   

        cmd1.ExecuteNonQuery();    
        con.Close();    
    }    
    catch (Exception ex)
    {
       throw ex; //TODO: Please log it or remove the catch
    }
    finally
    {
       con.close();
    }

}


try       
{           
    string query="my query";           
    db.run_runcommand(query);          
}         
catch(Exception ex)            
{         
    MessageBox.Show(ex.Message);              
}   
finally
{
   con.close();
}
Gilad Green
  • 36,708
  • 7
  • 61
  • 95
Freelancer
  • 9,008
  • 7
  • 42
  • 81
  • 1
    However, the `using-statement` is like a finally-block. So that does not make the difference. – Tim Schmelter Nov 13 '12 at 10:51
  • @TimSchmelter: I have never used using-statement in my projects. So, do using statements closes connections automatcally? – Freelancer Nov 13 '12 at 13:04
  • @freelancer: Yes, that's what i wanted to say in my answer(_"You should always close a connection immediately as soon as you're finished with it, best by using the using-statement:"_). So `using` is simply calling `dispose` at the end(even in case of an exception). `Connections` are definitely closing themself on `dispose`. Actually it's good practise to use `using` on every object implementing [`IDisposable`](http://msdn.microsoft.com/en-us/library/system.idisposable.aspx) because it might contain unmanaged resources(f.e. also your `SqlCommand`). – Tim Schmelter Nov 13 '12 at 13:09
  • @TimSchmelter: Ohh. Thanks for providing me new info. Then what was happening in Naeem Shah's case? It wasnt working? – Freelancer Nov 13 '12 at 13:14
  • @freelancer: I don't know, i assume he hasn't wrapped all in a using statement but stayed with his connection field. Always create/dispose the connection where you use it(in a method). – Tim Schmelter Nov 13 '12 at 13:20
  • I tried using statement, I still got this error: The connection was not closed the connection's current state is open. I thought using is supposed to close it? – user1663380 Sep 05 '13 at 08:42
  • Before I perform every sql statement, I will check if the connection is up, by using statement then try { connectionStr.Open()}catch(Exception){} – user1663380 Sep 05 '13 at 08:42
11

Check the connection state before opening it:

if (con.State != ConnectionState.Open)
    con.Open();
Shadow The GPT Wizard
  • 66,030
  • 26
  • 140
  • 208
4

A little more than the answers already here, I check if it's not just open, but connecting, and wait if it's in the connecting state.

if (con.State != ConnectionState.Open && con.State != ConnectionState.Connecting) {
    con.Open();
}
var attempts = 0;
while (con.State == ConnectionState.Connecting && attempts < 10) {
    attempts++;
    Thread.Sleep(500);
}

Of course you also need to put your con.Close() in a finally after try if you want to ensure your connection does get closed, since any code after the exception not in the finally doesn't get run.

Also you don't need to throw ex in your throw, you can just throw; by throwing ex you damage the stack trace.

Update: Looking over this I see you have con.Close() in your try AND in your finally - this will always error since if the try block works properly it will close the connection then move to the finally block which runs error or not, and try to close an already closed connection. Also be careful with your case - Close() and close() are not the same thing, only one actually calls the function, the other will error as it is undefined.

You need to remove the close from the try block and leave it only in the finally.

Andrew
  • 1,544
  • 1
  • 18
  • 36
2

Your connection string is opened. You can use code to check it:

if(cmd.Connection.State != ConnectionState.Open) cmd.Connection.Open();
Hiệp IT
  • 21
  • 1