piskvorky / sqlitedict

Persistent dict, backed by sqlite3 and pickle, multithread-safe.
Apache License 2.0
1.16k stars 130 forks source link

bug on commit() #108

Open acortiana opened 4 years ago

acortiana commented 4 years ago

I'm using sqlitedict version 1.6.0 and Python version 3.6.9 If I do this

mydict = SqliteDict('./my_db.sqlite', autocommit=False)
mydict['some_key'] = 'some_value'
mydict.commit()

and the commit fails for some reason, the mydict.commit() line hangs and never returns. I think that the problem is here, maybe a try/except is needed, with some error handling code: https://github.com/RaRe-Technologies/sqlitedict/blob/39e8dced2f56498df6e0d58a0addf42297917d3f/sqlitedict.py#L404

I tried to add a try/except and, in my case, I got this error:

2020-03-06 14:56:45 ERROR    Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/sqlitedict.py", line 405, in run
    conn.commit()
sqlite3.OperationalError: database is locked
jonchun commented 4 years ago

I just tested the same code and it works perfectly fine on my end.

sqlite3.OperationalError: database is locked

is most likely being thrown because you have a different process/thread trying to commit to the same sqlite db at the same time. Try completely restarting your computer (to make sure any orphaned/backgrounded tasks get killed) and see if you can reproduce the issue consistently.

Additionally see if you can reproduce the issue even after changing the db file name so you're 100% sure nothing else can be writing to the db

acortiana commented 4 years ago

The problem is not to understand why I'm getting the exception. In my use case, I have a lot of processes accessing the DB in the same time, and this is by design.

The problem is that sqlitedict is not responding in the right way when there is an exception on the underlying commit operation. To do a test in the right way, you have to make the underlying commit https://github.com/RaRe-Technologies/sqlitedict/blob/39e8dced2f56498df6e0d58a0addf42297917d3f/sqlitedict.py#L404 raise an exception. IMHO, the expected behavior is to make that exception "propagate" from the thread to the main process and be raised by mydict.commit() call

The current behavior is to hang indefinitely without ever returning and this cannot be the right behavior, right?

shantanoo commented 4 years ago

Quick fix could be adding timeout to sqlite connection.

https://stackoverflow.com/questions/2598801/python-sqlite-database-locked-despite-large-timeouts#3659261

ed2050 commented 3 years ago

Was anything ever done to fix this? The "quick fix" doesn't appear possible because there doesn't seem to be a way to pass sqlite connection params to sqlitedict. The connection happens inside sqlitedict.

Maybe what's needed is another arg to sqlitedict constructor. Something like (dict) connect_params, a dictionary of arguments for sqlite3.connect. Then pass it to connect as kwargs.

I don't see any code in sqlitedict.commit () that would swallow an exception. My guess is that acortiana did something more besides just wrapping conn.commit () in try / except. Although it seems he's using the multithread class based on line number, so perhaps it's a thread issue (exception gets raised and silently swallowed, or error message output is ignored, in a secondary thread, but error propagates all the way to the top when testing with one thread).

acortiana commented 3 years ago

I did this test to better demonstrate the problem: 1) added lines

if hasattr(self, 'test123'):
    raise NameError('HiThere')
self.test123 = 1

just after this line: https://github.com/RaRe-Technologies/sqlitedict/blob/39e8dced2f56498df6e0d58a0addf42297917d3f/sqlitedict.py#L404

2) run exactly this code

from sqlitedict import SqliteDict
mydict = SqliteDict('./my_db.sqlite', autocommit=False)
mydict['some_key'] = 'some_value'
mydict.commit()

I added the "hasattr" condition to skip the first commit from the "HiThere" exception. This because when the mydict object is created, it does an implicit (first) commit operation to create the table.

Executing the code, you can see that the Thread-1 raises the exception but you can also see that mydict.commit() command remains stuck without ever returning. IMHO the right behavior is that mydict.commit() also must raise an exception, without remaining stuck forever.