search

FSX / momoko

Wraps (asynchronous) Psycopg2 for Tornado.
http://momoko.61924.nl/
Other
364 stars 73 forks source link

code in ConnectionContainer.all_dead conflicts with Pool.putconn #139

Closed masknu closed 8 years ago

masknu commented 8 years ago

Below two lines seem conflict with each other: return not (self.free or self.busy or self.waiting_queue) self.conns.abort_waiting_queue(self._no_conn_available_error)

If there is something in waiting_queue need to be aborted, then if self.conns.all_dead: will return False. Thus the self.conns.abort_waiting_queue is not going to get executed.

The possible solution: Change return not (self.free or self.busy or self.waiting_queue) to return not (self.free or self.busy or self.pending)

haizaar commented 8 years ago

Looks like you are correct. How did you spot it? Do you have a minimal reproduction code that can be put to unittest?

masknu commented 8 years ago

I just looked into the unittest code. I guess we could spot this issue by adding a line follows L816 in test_abort_waiting_queue():

self.assertIsInstance(f2.exception(), momoko.Pool.DatabaseNotAvailable)

Not tested yet. Do you guess the same way?

masknu commented 8 years ago

But with the execution order of L478 and L479 in Pool._operate:

                if not keep:
                    self.putconn(conn)
                future.set_result(result)

I guess the second db query could get the only connection and issue the query command successfully. Then the testcase disconnects db by send SIGTERM to tcproxy. I consider it highly likely that the second db query could finish before the connection disconnect. Which would result in altered version of test_abort_waiting_queue()'s failure. Maybe we need a better solution in unittest.

haizaar commented 8 years ago

You are right both ways - I've changed the code to set the result first to let a done callback in the unittest to kill the connections. Then the assertion work (after fixing the original bug of course).

For the test with proxy, however the test will not raise DatabaseNotAvailable, because restarting proxy will not immediately mark the connection object as closed. I guess we'll have to live with it for now.

See https://github.com/FSX/momoko/pull/142. What do you think?