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

things-cli fails with "sqlite3.OperationalError: unable to open database file" when listing a large number of todos (~2500) #125

Open
mbhutton opened this issue Oct 27, 2024 · 5 comments

Comments

@mbhutton
Copy link
Contributor

Note: solved, with an upcoming pull request, but filing here as a bug report for reference.

To Reproduce
Steps to reproduce the behavior:

  1. Create 2500 or so tasks (don't ask 😬)
  2. Run things-cli todos

Expected behavior
It runs successfully

Observed behaviour
It fails with the following:

Traceback (most recent call last):
  File "/Users/matt/.local/share/mh/uv-tool-bin/things-cli", line 8, in <module>
  File "/Users/matt/.local/share/uv/tools/things-cli/lib/python3.13/site-packages/things_cli/cli.py", line 487, in main
  File "/Users/matt/.local/share/uv/tools/things-cli/lib/python3.13/site-packages/things_cli/cli.py", line 406, in main
  File "/Users/matt/.local/share/uv/tools/things-cli/lib/python3.13/site-packages/things_cli/cli.py", line 422, in main
  File "/Users/matt/.local/share/uv/tools/things-cli/lib/python3.13/site-packages/things_cli/cli.py", line 479, in parse_command
  File "/Users/matt/.local/share/uv/tools/things-cli/lib/python3.13/site-packages/things/api.py", line 458, in todos
  File "/Users/matt/.local/share/uv/tools/things-cli/lib/python3.13/site-packages/things/api.py", line 206, in tasks
  File "/Users/matt/.local/share/uv/tools/things-cli/lib/python3.13/site-packages/things/database.py", line 395, in get_tags
  File "/Users/matt/.local/share/uv/tools/things-cli/lib/python3.13/site-packages/things/database.py", line 429, in get_tags_of_task
  File "/Users/matt/.local/share/uv/tools/things-cli/lib/python3.13/site-packages/things/database.py", line 500, in execute_query
sqlite3.OperationalError: unable to open database file
things-cli todos  0.09s user 0.02s system 61% cpu 0.178 total

Python: 3.13.0
things-cli: version 0.2.1

Root cause appears to be that each SQL query creates a new connection which is never closed, and so when querying tags for each task, the number of open connections is at least as high as the number of tasks.

Working fix is to close the connection each time after each query.

mbhutton added a commit to mbhutton/things.py that referenced this issue Oct 27, 2024
Each query creates a new DB connection.
This change is to close each new connection rather than leaving
them open, to allow for runs which involve large numbers of queries.
AlexanderWillner pushed a commit that referenced this issue Nov 10, 2024
Each query creates a new DB connection.
This change is to close each new connection rather than leaving
them open, to allow for runs which involve large numbers of queries.
@AlexanderWillner
Copy link
Contributor

Actually, the tests with Python 3.13 warn about:

things/database.py:512: ResourceWarning: unclosed database in <sqlite3.Connection object at 0x101f0cf40>
cursor.execute(sql_query, parameters)
Object allocated at (most recent call last):
File "things/database.py", lineno 191

@AlexanderWillner
Copy link
Contributor

@mbhutton
Copy link
Contributor Author

mbhutton commented Nov 14, 2024

Thanks, in that case I think it'd make sense to roll back the second of those commits, as the intention of that second commit was as an optional performance improvement to leave the connection open (separate transactions, but shared connection), and that warning makes it clear that it's bad practice to do so.

I.e. this later of the commits would need to be rolled back:
bf429cd

I tested both versions (with just the first commit and with both), and while sharing a connecting did speed things up (75% or so faster IIRC), it's already very fast even on large Things databases.

Would you prefer a PR for the revert of that second commit above, or prefer to revert it directly?

Btw thank you for this great tool!

@AlexanderWillner
Copy link
Contributor

Thanks. A PR that make sure that make test does not throw any warnings would be nice.

@mikez
Copy link
Contributor

mikez commented Dec 3, 2024

@mbhutton @AlexanderWillner Thank you for your work here. 75% faster! 🥳

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

No branches or pull requests

3 participants