adlnet-archive / 3D-Repository

The ADL 3D Repository is a website for uploading, finding, and downloading 3D models.
25 stars 16 forks source link

ODBC connections need to close properly #2

Closed rchadwic closed 12 years ago

rchadwic commented 12 years ago

The sql server really should not need 2000 connections. It looks like the ODBC connector is not closing them correctly. Is there an updated MySQL ODBC connector?

AustinMontoya commented 12 years ago

Which tool were you using to do the analysis (Workbench/MySQL Administrator/etc.)? What browser requests were you making, how many connections did they open each (approx.), and did all of them close once the request completed or stick around, eventually causing mysql to hang?

rchadwic commented 12 years ago

This was on the IMI upgrade. The permissions create a lot of calls to the database, and that eventually overwhelmed their max_connections. Our server uses 2000 as the max, and that seems to be enough, but the IMI probably has the default... Not really sure whats going on, but I know we had some issue with this in the past.

AustinMontoya commented 12 years ago

Ah, I see. Much of the DAL refactoring over the last few months was indeed related to reducing the number of connections needed to successfully retrieve permissions for large datasets (e.g. CreatePermittedObjectsTable stored proc). I will do some experimentation with various calls to our 3DR instance to evaluate the connection count and report back here. Thanks for the clarification!

On Thu, Feb 2, 2012 at 10:54 AM, rchadwic < reply@reply.github.com

wrote:

This was on the IMI upgrade. The permissions create a lot of calls to the database, and that eventually overwhelmed their max_connections. Our server uses 2000 as the max, and that seems to be enough, but the IMI probably has the default... Not really sure whats going on, but I know we had some issue with this in the past.


Reply to this email directly or view it on GitHub: https://github.com/adlnet/3D-Repository/issues/2#issuecomment-3780225

Austin Montoya Web Developer Joint ADL Co-Laboratory

Office: 407-384-5144 Cell: 407-451-0849 Skype: austin.montoya.ctr

AustinMontoya commented 12 years ago

Did an evaluation on two pages, /Public/Results.aspx?Group=rating-high and Public/Model.aspx?ContentObjectID=adl:670. With Results, where considerable refactoring was done to speed up search, the connection count never raised above 1 per user per request, regardless of whether 1 or 100 results are returned, so I don't think we have too much of an issue there. On Model, however, it was opening 7 connections per user per request(!). Clearly this is where we need to focus on improvements. First step might be to trace the connections and what is/isn't being reused when building our response.

rchadwic commented 12 years ago

yeah, I really think that the using(odbc....) construct does not close the connections. I think we should go through the DAL and make sure to explicitly close them.

AustinMontoya commented 12 years ago

DbConnection (the parent class of OdbcConnection) implements the IDisposable interface, so the using statement should work as intended. Even the MSDN example for the class has a using statement (http://msdn.microsoft.com/en-us/library/system.data.odbc.odbcconnection.aspx). I tend to think more that it's probably fedora commons or something else.

rchadwic commented 12 years ago

Here's an interesting read on the topic. http://www.primaryobjects.com/CMS/Article69.aspx- looks like there really is a bug in the driver. I followed advice here, but seems like a clunky solution. I'm refactoring the DAL, APIKeyManager, and PermissionManager to kill their own processes using the method described here ( SQL kill connectionID()). Also, Making them disposable, adding destructors. All connections will be for the life of the object, instead of a new one per function.

AustinMontoya commented 12 years ago

I agree with the part about refactoring so that connections are re-used for an object's lifespan as good practice, as it will likely reduce the number of connections made on a Model.aspx request, which is a valid fix to this issue. However, I still don't think there are sleeping connections.

When I did my evaluation mentioned above, I did not see one connection stay around for longer than the lifetime of the request, ever, but you may have different findings. This is fairly easy to reproduce:

  1. Open up MySQL Administrator and connect to the database you are working with.
  2. Use jmeter or ab to simulate normal database traffic on the most visited urls (I chose Results and Model, you may want to run a complete set).
  3. while ab or jmeter are executing, look at the connections tab in MySQL Administrator. You should see a rise, then fall in connections back to 1 (MySQL Administrator itself uses a sleeping connection to monitor the database, so there's always at least 1 while the program is running). Also, there's another tab that shows who is connecting to the database that you may want to monitor as well.

If there is more than one connection still open after the requests have been properly served by the application, then we do in fact have a connection problem. If it drops back to the state it was in before you made the requests, then there are no sleeping connections. I am currently unaware of whether MySQL Admin would not catch sleeping connections within its interface, but it seems pretty doubtful.

The only reason I am so cautious to just integrate the solution posed above is because the article was written in 2006 and may no longer be relevant. Also, like you said, it is clunky and potentially adds unnecessary complexity to our application. Finally, unless the production code is doing something different I don't believe we are using connection pooling either (though it may help), which is where the article mentions a flaw in the driver. Please take these considerations into account.

rchadwic commented 12 years ago

Humm, is pooling not enabled by default? So, I've gone through and made the connections live for the lifetime of each object, rather than one for each function. I also setup the DAL and such as Disposable, and made sure to go through the code and call dispose where necessary. Oddly, I can recreate this issue on my local dev machine, but not on the stage server. On my local box, the "kill connection_id()" trick works nicely in the dispose function for the DAL, and if I change the DAL dispose to call mConnection.close(); mConnection.Dispose, I can clearly see sleeping threads in MySQL Administrator. The above changes do solve the problem, but now I'm not certain I can recreate the problem. It would be nice if we could just call mConnection.close() on the dispose method. I'll keep poking to see if I can narrow down the issue. It's possible that going through and adding all the Dispose calls closes a connection that we had simply missed.

rchadwic commented 12 years ago

So, here's an interesting bit - I now have these dispose methods, so that's a good place to test things.

Leaves connections open forever

void Dispose()
{
     mConnection.Dispose();
}

Closes properly, issues when ASP pages don't call dispose for DAL

void Dispose()
{
      mConnection.Close();
      mConnection.Dispose();
}

So, how did the using statement even close the connections? I'm sort of in a point where I can break it on purpose, fix it on purpose, but not entirely sure why it worked or did not before.

rchadwic commented 12 years ago

So, it's actually not that line of code at all. Neither of those makes a difference - under my install of IIS, the connections don't close, but under the dev server they do. No idea whats going on.

rchadwic commented 12 years ago

Think the self killing connections got it. Austin is running performance tests.