search

clueboy / pymssql_issues

0 stars 0 forks source link

connections are not closed automatically because pymssql keep a strong reference on all connections #107

Closed clueboy closed 11 years ago

clueboy commented 11 years ago

From guillaume.pratte on January 08, 2013 10:18:52

I am having an issue with the usage of the connection_object_list in _mssql.pyx.

This is a list that is used in msg_handler() in order to obtain the error messages.

The problem I have is the following. Since the global connection_object_list is keeping a strong reference on all connections, when a connection is no longer referenced, it does not get deleted and the socket to the database stays opened.

This situation can lead a program to have too many opened files if the close() method is not explicitely called (this is causing me some troubles with unit tests).

Could it be possible to consider using a weak ref list instead, so the connection would get deleted when no longer referenced?

Original issue: http://code.google.com/p/pymssql/issues/detail?id=107

clueboy commented 11 years ago

From guillaume.pratte on January 08, 2013 10:45:52

Here is a suggested implementation of this idea.

Attachment: 107-weakref.patch

clueboy commented 11 years ago

From guillaume.pratte on January 08, 2013 13:35:35

Here is an updated version of the patch.

Attachment: 107-weakref.patch

clueboy commented 11 years ago

From msabramo on August 10, 2013 13:16:56

Thank you for this patch! At a glance, this looks good. I'll give it a more thorough look sometime, but it might take a while because I'm on vacation now for 2 weeks.

Status: Accepted
Owner: msabramo
Labels: Leak

clueboy commented 11 years ago

From msabramo on August 10, 2013 13:19:34

Labels: Needstesting

clueboy commented 11 years ago

From msabramo on August 10, 2013 13:51:42

https://code.google.com/p/pymssql/issues/detail?id=88 appears to be the same issue, so I think that the patch here will probably allow us to close TWO bugs. :-)

clueboy commented 11 years ago

From msabramo on August 10, 2013 14:06:01

Cc: sokann

clueboy commented 11 years ago

From guillaume.pratte on August 11, 2013 07:44:35

Great! Thanks for reviewing the patch!

I have been running this patch in production since January, and so far it has been proven to be stable.

If there is anything to be changed in the patch, let me know!

clueboy commented 11 years ago

From msabramo on August 13, 2013 09:57:01

From a quick experiment last night, it appears that this patch might not be needed after this commit: https://code.google.com/p/pymssql/source/detail?r=54554641cc7e487b9e8d5aa63b7085bc31a5f060 The above commit makes it so that connections actually get closed and removed from connection_object_list when they are out of scope.

Can you test it out?

clueboy commented 11 years ago

From msabramo on August 13, 2013 20:05:38

As I mentioned previously, I don't know whether this weakref change is necessary when connections deallocate properly, but I tweaked the earlier patch a bit for style (rename self.weakref to self._weakref to reflect its private nature) and to prevent a compilation error that I was getting with Cython 0.19.1: https://gist.github.com/msabramo/6223415 Will hold off on applying this to see if it's necessary.

clueboy commented 11 years ago

From msabramo on August 13, 2013 20:29:02

Status: Fixed