Skip to content
This repository has been archived by the owner on Sep 24, 2022. It is now read-only.

fix for any uncatchable internal exceptions #134

Closed
wants to merge 2 commits into from

Conversation

m-messiah
Copy link

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
Copy link
Collaborator

haizaar commented Nov 17, 2015

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

@m-messiah
Copy link
Author

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
Copy link
Author

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
Copy link
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 zarmory/momoko@7dcc245? I've fixed it there.

@m-messiah
Copy link
Author

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
Copy link
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 closed this in 7dcc245 Nov 30, 2015
friedcell added a commit to friedcell/momoko that referenced this pull request Jul 13, 2023
* commit 'ff4b2189f3a9f6127167678204f58a42cf93a9d7':
  Going 2.2.2
  PEP8 fix
  Catching all syncronous errors. Fixes FSX#134.
  Prevening tornado from logging (falsy) errors
  Update README.rst
  Fix spelling error (renames an instance attr).
  Fix another docstring typo.
  Grammar/typo fixes in a docstring.
  docs: clarify an ambiguous paragraph
  Catching IOError. Fixes FSX#127
  Fix typo in readme.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants