coleifer / peewee

a small, expressive orm -- supports postgresql, mysql, sqlite and cockroachdb
http://docs.peewee-orm.com/
MIT License
11.11k stars 1.37k forks source link

Unable to except OperationalError / connection trying to be closed after catch block #1093

Closed theotherp closed 7 years ago

theotherp commented 7 years ago

This is what my code looks like:

try:
    searchResult, _ = tryGetOrCreateSearchResultDbEntry(indexer.indexer.id, result)
    result.searchResultId = searchResult.id
    search_results.append(result)
except (IntegrityError, OperationalError) as e:
    logger.error("Error while trying to save search result to database. Skipping it. Error: %s" % e)

and this the stacktrace:

2016-10-08 16:05:21,416 - ERROR - web - cannot commit - no transaction is active
Traceback (most recent call last):
  File "/home/snowcrash/nzbhydra/nzbhydra/web.py", line 570, in api_search
    result = search.search(search_request)
  File "/home/snowcrash/nzbhydra/nzbhydra/search.py", line 318, in search
    logger.error("Error while trying to save search result to database. Skipping it. Error: %s" % e)
  File "/home/snowcrash/nzbhydra/libs/peewee.py", line 4314, in __exit__
    return self._helper.__exit__(exc_type, exc_val, exc_tb)
  File "/home/snowcrash/nzbhydra/libs/peewee.py", line 4348, in __exit__
    self.commit(False)
  File "/home/snowcrash/nzbhydra/libs/peewee.py", line 4325, in commit
    self.db.commit()
  File "/home/snowcrash/nzbhydra/libs/peewee.py", line 3711, in commit
    self.get_conn().commit()
OperationalError: cannot commit - no transaction is active

So some part of the code magically tries to commit and fails. I'd like to ignore the OperationalError (as I'm unable to completely prevent it) that happens while trying to access the db but can't catch that exception, then it's raised in the catch block. I even tried disabling autocommit before and committing manually but even then the error occurred.

Edit: I managed to get rid of all errors by using a thread lock for the main part of the code that writes to the database. It increases the run time but that's worth it. I leave it up to you to close this or investigate further.

coleifer commented 7 years ago

OK, I found a bug in Peewee where the calls to commit and rollback weren't being wrapped in the exception wrapper that converts driver-specific exceptions into peewee exceptions. So that is fixed in 900ed22ee5505fa56565d52dc22cfbce496ec60f

coleifer commented 7 years ago
OperationalError: cannot commit - no transaction is active

This exception, with SqliteQueueDatabase, is what I tried to warn folks about when using this database class. If you try to use transactions in multiple threads at the same time, there is a risk of interleaving that will yield exactly this kind of error. For example:

Or the opposite:

coleifer commented 7 years ago

I managed to get rid of all errors by using a thread lock for the main part of the code that writes to the database. It increases the run time but that's worth it. I leave it up to you to close this or investigate further.

If you're going to use the SqliteQueueDatabase, then you're responsible for managing your transactions such that these kinds of things don't happen. Because with the way it's designed to work, it's quite easy to shoot yourself in the foot. I strongly suggest reading the code to get an understanding for how it works. I also strongly suggest reading the isolation documentation on sqlite's website:

http://sqlite.org/isolation.html

This will help you understand why things happen the way they do when you have multiple threads operating on a single database.

theotherp commented 7 years ago

You wrote in the other thread I shouldn't be using multi-statement transactions. Does that mean that without using db.atomic() this wouldn't happen? I read your code and tried to understand it. I generally try to do some research and experiments before bothering others (although it might seem otherwise ;-)) but I got nowhere. I tried to replicate my problems in an easier program but was unable to do so (probably because I don't understand it correctly).

Anyway, thank you; I will do some reading. I never knew what I got into when I introduced a database in my tool...

coleifer commented 7 years ago

So the issue was this, as I understand it.

Each database driver (sqlite3, pymysql, psycopg2, etc) implements all or most of the DB-API 2.0 spec (PEP-249). This PEP outlines a handful of exception types. What Peewee does is it wraps the vendor-specific exception classes so that you only need to worry about catching the exceptions defined in Peewee itself. This allows portability between db implementations, e.g., I can catch peewee.OperationalError instead of having to catch sqlite3.OperationalError or psycopg2.OperationalError. Unfortunately, though, there was no exception wrapping taking place on the methods where Peewee was calling "commit" and "rollback" directly on the database driver. This meant that if you called commit and it triggered an OperationalError, you would get the "vendor-specific" version as opposed to the peewee version. That bug is what the above commit, 900ed22, fixes.

theotherp commented 7 years ago

Hm. I think I tried to catch the specific OperationalError and it didn't help. But doesn't matter, it seems to be mostly fixed by doing some locking and so on. Thanks for the help.

coleifer commented 7 years ago

You wrote in the other thread I shouldn't be using multi-statement transactions. Does that mean that without using db.atomic() this wouldn't happen?

The raison d'être for db.atomic() is to allow multi-statement transactions. So, it would follow that this isn't going to play nicely with SqliteQueueDatabase. The reasons are described above, but have to do with the fact that behind-the-scenes there is only one connection writing to the database despite there being the appearance of multiple concurrent writers. I feel like I've explained this numerous times, is there something that's unclear about how it works?

theotherp commented 7 years ago

No, I wanted to be sure I got it. Thanks for the patience ;-)