Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix pool bug #113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix pool bug #113

wants to merge 1 commit into from

Conversation

btmorex
Copy link

@btmorex btmorex commented Jul 26, 2014

This fixes a bug where in certain circumstances greenlets can get blocked indefinitely in pool.get() even though there are items available (or at least current_size < max_size). To trigger this bug, the following conditions must be met:

1.) There are greenlets waiting on items from the pool (pool.waiting() > 0)
2.) pool.create() must throw an exception
3.) pool.get() must not be called again (or more accurately, greenlets will block until pool.get() is called again)

It seems like those would be fairly rare circumstance, especially 3.), but our use of pool triggers this any time we have a database failover. We maintain two different pool's corresponding to database servers db1 an db2. Normally, we would just use the pool of connections to db1. Suppose that db1 goes completely down (no longer responds to network requests):

1.) We'll almost certainly have pool.waiting() > 0 because queries that were taking milliseconds, now must wait on a timeout in the seconds range
2.) pool.create() will throw an exception (some kind of connect timeout)
3.) As soon as we get a network exception, we failover and start using db2's pool and don't call get() on db1's pool again.

So basically, any failover scenario cause a greenlets to block indefinitely.

This fix has two caveats:

1.) You can no longer have a pool of None objects since they now have special meaning. This was already the case for db_pool
2.) If there are greenlet's waiting and create() throws an exception we lose exception info because put() causes a greenlet switch. We raise the exception again, but it now appears to originate in pool itself.

@temoto
Copy link
Member

temoto commented Aug 11, 2014

Thank you very much. This is a terrific fix, though again, needs a test.

I think I can fix caveats:

  1. for None, I often use _MISSING = object() idiom, first seen it in Werkzeug.
  2. for exception origin, we have six.reraise(*sys.exc_info())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants