fix for any uncatchable internal exceptions #134

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@m-messiah

There is the problem, when momoko fails with exception, such as, for example, IndexError for parameters, which is not catchable by Tornado. In the result of this behavior - this connection stays busy and inoperable. So, this little patch would catch these exceptions and throw them to Tornado

@haizaar
Collaborator
haizaar commented Nov 17, 2015

Thanks for reporting the issue. Can you please the working reproduction? Ideally, a failing unittest?

@m-messiah

So, when this case is so strange and rare, and maybe you can say - this code is bad for using - problem still exists - little bug in one of threads of pool can produce uncatchable exception and crash all app.

@m-messiah

Ok, then, RuCTFE 2015 is over and I can show you piece of code, which fails here. (except unittest, published earlier).
Here, https://github.com/HackerDom/ructfe-2015/blob/master/services/mol/service/main.py#L458 , we have sql injection (in unittest I had written better way, when it may be needed), which works perfectly except one input:

when variable 'offset' is sended by another python code, where it looks like

offset = "0; SELECT * FROM users WHERE username LIKE '%%='"

where %% used to prevent string interpolation.

So, when this input received - this thread of momoko.Pool throws uncatchable exception and doesn't close till application restart.

@haizaar
Collaborator
haizaar commented Nov 30, 2015

I think I see what you are saying - cursor.execute can throw non psycopg2.Error based exception as well. The proper was to fix it is on the Pool level - if you are using Connection stand-alone, the error is propagated just fine to the caller.

Can you try my master branch haizaar/momoko@7dcc245? I've fixed it there.

@m-messiah

My environment shows a lot of failed tests, so I can't recognize is problem still exists, or it is gone((

The main test, which failed for me is:

search = "WHERE name LIKE '%%searchable' LIMIT %s" % 10
sql = yield self.conn.execute("SELECT COUNT(*) FROM unittest_large_query %s;" % search)

Is it still fails, or your fix repaired it? I see, your test is about some different case, which was catchable for me earlier.

@haizaar
Collaborator
haizaar commented Nov 30, 2015

My fix works for your case as well - they both raise IndexError. Mine version is dead stupid to outline that we pass malformed string deliberately.

I'm merging mine and closing this then.

Thanks for your time to find it out!

@haizaar haizaar added a commit that closed this pull request Nov 30, 2015
@haizaar haizaar Catching all syncronous errors. Fixes #134.
Things like `cursor.execute` can throw not only `psycopg2.Error`
exeception and their derivatives, but also standard Python errors
like `IndexError` bacause badly formatted SQL. In the latter case,
after such exception the connection became stale (i.e. forever-busy)
in the Pool.
7dcc245
@haizaar haizaar closed this in 7dcc245 Nov 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment