0

I have an ASP.NET Web API that uses an SqlConnection to connect to a database. I have a data access layer class which has an instance variable containing the connection. I do this for a couple of reasons:

  1. the calling code can override the connection string in the constructor of the DAL class (e.g. for test code)
  2. there are some cases where the API controller needs to open an SQL connection, begin a transaction, then call several methods in the DAL class before committing (or maybe rolling back) the transaction. So, closing and re-opening the connection per method will not work because I have to hold the connection open (and even keep the SqlTransaction object in scope - I'd do that by making it also an instance variable) in order to not have the transaction rolled back between DAL calls.
  3. It also simplifies code readability, so you're not duplicating code all over the place to open SQL connections.

When I stress-tested my API, by feeding it hundreds of requests per second, I hit up against the SQL connection pool exhaustion issue. Further investigating shows that this appears to be because the SQL connection is not being disposed of.

I do understand the IDisposable pattern, but I am not sure how I would use it in this scenario. Here's my problem:

  1. Using the using block, or the try/catch/finally block, both require the object to be created, used and finalized within a single method. In the example above of a transaction that may need to persist across multiple method calls, this is not possible.
  2. Microsoft (and other posts on SO) has recommended against simply putting a call to Dispose() in an object destructor, instead suggesting you do either of the options I specified in problem 1.
  3. Microsoft also says you shouldn't implement IDisposable yourself just to wrap around another manage object's Dispose method, but instead you should "simply call Dispose() on the object when you're finished with it." How would I do this in this scenario when I am not sure from within the DAL when the controller is finished using the SQL connection? (Also, this would mean I have to refactor the controller to wrap each call to the DAL in a using block, so it's just kicking the can down the road.)

The ideal solution for me would be to be able to somehow arrange to have the Dispose method called on the SqlConnection object when the controller has finished processing and is returning its response to the server for delivery to the frontend. To do this "by hand" I would have to violate point 3 above and create my own Dispose method on my DAL that simply in turn calls the SqlConnection's Dispose. Also it would mean I have to refactor many methods in all controllers to wrap all DAL access in using blocks. It appears that ASP.NET will not automatically call Dispose when the controller returns, which is why the connections are leaking.

In either case, it also makes for more verbose code. For example:

// we only need one method call from the DAL, so let's be compact
string someData = new DAL().GetSomeData(someParam);

now has to be written out as:

// we have to initialize here to keep the variable from falling out of scope after the using blocks
// we also must provide some value because the using block implies try/catch.
string someData = "";
using (DAL d = new DAL()) {
    someData = d.getSomeData(someParam);
}

What would be the recommended way to implement this?

On a more generic plane, how do you actually deal with a disposable object that must persist between method calls (e.g. as an instance variable)? The need to use disposables within constructs like try/catch/finally or using seems to limit their use only to situations where the object can be created and disposed within a single method.

fdmillion
  • 4,823
  • 7
  • 45
  • 82
  • 1
    If you are using DI container - you can let it dispose your resources when request ends. Though I personally prefer to manage connections myself - open when I need, do stuff, close. – Evk May 31 '17 at 06:04

2 Answers2

0

As MSDN recommends you should use the

using(var cn = new SqlConnection(xx)){ cn.Open(); }

NOTE: it does NOT open a new connection each time

The SqlConnection draws an open connection from the connection pool if one is available. Otherwise, it establishes a new connection to an instance of SQL Server.

And in order to controll Your transactions - You can just use the TransactionScope() (although it may require MSDTC in some case`s)


Otherwise, if using TransactionScope is NOT an option, and You want to explicitly control Your transactions, the only option is to pass the connection along. As it is discussed in this POST

You can do the "passing along" in 2 ways. Manually or use Dependency Injection (DI). All of the DI containers allow You to control the lifetime of the instance. So in Your case it might be a "per request lifetime" a sample can be found in this post

Marty
  • 3,485
  • 8
  • 38
  • 69
  • This true most of the time but you need to make sure that the connection string does not disable connection pooling, it's not the default behavior but it can happen. – Zalomon May 31 '17 at 07:42
  • Based on these answers it seems there's no way to do this at least without TransactionScope... This is too bad, it seems to defeat the entire point of separating the DAL into a separate class. As soon as the Web API must manage creating and disposing connections, you lose the benefit of having the DAL separate (e.g. being able to replace the DBMS simply by writing a new DAL, or even using a mocked DAL for testing). It looks like the cleanest option may be to simply make the DAL a disposable and then do `using`s in the Web API for the DAL, even if that's against the "recommended" practice. – fdmillion May 31 '17 at 13:49
  • The post you reference describes my exact issue: "Sometimes it helps to elevate the connection to a member of the class in which you're doing the work, but this can create awkward situations - and complicates controlling the lifetime and disposal of the connection object (since it generally precludes use of the using statement)." So the only way I can do this is to make the DAL disposable and wrap it in a `using` in each controller method... – fdmillion May 31 '17 at 13:55
0

Regarding this paragraph.

On a more generic plane, how do you actually deal with a disposable object that must persist between method calls (e.g. as an instance variable)? The need to use disposables within constructs like try/catch/finally or using seems to limit their use only to situations where the object can be created and disposed within a single method.

As a rule of thumb the method responsible for instantiating a disposable object should be the one responsible of disposing it, it is ok to pass it along as an argument to other methods that are called from the instantiating methods but ,these methods, should not be responsible of disposing the objects. The exception to this rule might be if you instantiate the object to inject into another class, but in this case this class should implent IDisposable and should be disposed by the invoker after it is used.

Zalomon
  • 553
  • 4
  • 14
  • That makes sense, it just doesn't follow the logic model I'm using... As I said above, as soon as you require the Web API to create `SqlConnection` objects and pass them into the DAL, you lose any benefit of having a separated DAL (such as being able to completely replace the DBMS by simply replacing the DAL). The same could apply to other scenarios as well. DI certainly has its place but it does also sometimes violate the "component" idea, in this case, that you have one class that provides everything you need to get to the database... – fdmillion May 31 '17 at 13:52
  • Id's say that you're violating your design when the API instantiates SQLConnection sometimes instead of the DAL; matter of fact the API should not know anything about the existence of SQLConnection, you should have a common entry point to the database where connections are created and disposed – Zalomon May 31 '17 at 14:02