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

Closed
masknu opened this Issue Apr 20, 2016 · 4 comments

Projects

None yet

2 participants

@masknu
masknu commented Apr 20, 2016

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
Collaborator
haizaar commented Apr 20, 2016

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

@masknu
masknu commented Apr 21, 2016

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
masknu commented Apr 21, 2016

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
Collaborator
haizaar commented Apr 21, 2016

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 #142. What do you think?

@haizaar haizaar closed this in 069f199 May 10, 2016
@vladvoskoboynik vladvoskoboynik referenced this issue in haizaar/momoko Jul 3, 2016
@haizaar haizaar Fixes 139 - `all_dead` should check for pending
`all_dead` - should check for pending connections
and not waiting_queue.

To be able to test it properly, future's result
need to be set before connection is returned to
pool in order to allow our kill callbacks to run.
e64170f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment