Merge lp:~cedric-lebrouster/ocb-server/ocb-7.0-bug-1322191-db_maxconn into lp:ocb-server

[Cedric Le Brouster(OpenFire)]

Close database connections before removing them.
Fixes bug lp:1322191

[Pedro Manuel Baeza (pedro.baeza)]

LGTM. Thanks for the patch.

Regards.

review: Approve (code review)

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

LGTM : the connection must be closed.

But

I cannot find by reading the code a case where this branch is executed (i.e. a logical path in which we get len(self._connections) >= self._maxconn so I'm wondering if we are not improving dead code here.

Do you have a scenario at hand?

review: Approve (code review, sometest without being able to exercise that code)

[Cedric Le Brouster(OpenFire) (cedric-lebrouster)]

Thanks for review.

What I understand by reading the code is that this function has 4 parts.
First it removes closed or leaked connections - none of our business here
Then it tries to re-use an iddle connection previously opened for the same database
If no iddle connection is found for this database, and if db_maxconn is reached, it will free any other iddle connection, and here was the bug
Finally, it will open a new connection

So the bug needs to have several databases opened to occur.

How to test :
I check number of postgres connections on linux with the psql command:
psql -c "SELECT datid,datname,procpid,usesysid,usename,current_query FROM pg_stat_activity WHERE application_name!='psql';" postgres

Set in config file db_maxconn = 4 (must be higher than max_cron_threads if I remember correct)
Run openerp
Login to a database
You should see 3 or 4 postgres connections for your database.

Now, without loging out, login to another database (loging out would free connections)
You should now see more than 4 used databases. If not, click on a menu on the 2 databases with a low time interval. For whatever reason connections are correctly freed or their database switched sometimes, but I don't know why or where in the code.

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

thanks for the precision.

[Holger Brunn (Therp) (hbrunn)]

Development for 7.0 has moved to github on https://github.com/OCA/ocb - please move your merge proposal there if it is still valid.

(I close and reject this in order to have a cleaner overview for 6.1 MPs which indeed have to be done on launchpad)

review: Disapprove

Unmerged revisions

5343. By Cedric Le Brouster(OpenFire) on 2014-05-22

[FIX] sql_db: close db connexions before removing them. bug-1322191

=== modified file 'openerp/sql_db.py'
--- openerp/sql_db.py	2014-04-09 15:16:59 +0000
+++ openerp/sql_db.py	2014-05-22 14:50:38 +0000
@@ -430,6 +430,8 @@
             for i, (cnx, used) in enumerate(self._connections):
                 if not used:
                     self._connections.pop(i)
+                    if not cnx.closed:
+                        cnx.close()
                     self._debug('Removing old connection at index %d: %r', i, cnx.dsn)
                     break
             else: