Skip to content

Use a context manager for all write actions to prevent indefinite database locks #77

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

Merged
merged 5 commits into from
Apr 13, 2024

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Apr 12, 2024

#76 uses a unit test to expose an issue we see when a "write" to the database raises an exception. The database becomes locked indefinitely, preventing any other connects from talking to the database.

In this PR, I wrap all "write" actions with try/except clauses, log any errors, and free up the lock.

@Zsailer Zsailer added the bug Something isn't working label Apr 12, 2024
Copy link

codecov bot commented Apr 12, 2024

Codecov Report

Attention: Patch coverage is 56.25000% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 87.35%. Comparing base (ab2ee68) to head (f5f7b6b).

❗ Current head f5f7b6b differs from pull request most recent head 59ae22d. Consider uploading reports for the commit 59ae22d to get more accurate results

Files Patch % Lines
jupyter_server_fileid/manager.py 56.25% 12 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
- Coverage   88.71%   87.35%   -1.37%     
==========================================
  Files           6        6              
  Lines         567      585      +18     
  Branches       69       67       -2     
==========================================
+ Hits          503      511       +8     
- Misses         43       53      +10     
  Partials       21       21              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR! I will offer an explanation as to why you are encountering this error, and a proposal to handle this more appropriately.

There are three important pieces of context:

  1. The DB is locked if there are any pending (i.e. un-committed) transactions.
  2. In the current implementation, we only commit at the end of each public method for a performance benefit. This is because committing several transactions at once is faster than committing after each transaction, as each commit triggers a disk write op.
  3. However, in the main branch, if an exception is raised in a private method, then the public method calling the private method immediately exits, and the self.con.commit() line is never reached. This is means that after an exception is raised, the DB is still locked as there are pending transactions.

While your current implementation does fix the issue, it is essentially ignoring failed transactions and committing all the successful ones anyways. Ideally, each set of transactions produced by a public method are either all committed or all rolled back; no in-between. I expect this will prevent subtle DB integrity issues that are tricky to debug.

With this info, I believe this proposal would handle the error more correctly:

  1. Wrap all public methods in the Connection context manager, e.g.
def index(self, path: str) -> str:
    with self.con:
        ...
  1. Remove all calls to self.con.commit() at the end of each public method.

This means that public methods will still raise a sqlite3.IntegrityError exception if the operation was invalid, but should also ensure the DB is not locked after the exception is raised. I believe this is preferable to the current PR, which never raises the exception back to the consumer.

@Zsailer
Copy link
Member Author

Zsailer commented Apr 12, 2024

Thanks, @dlqqq! That all sounds good.

Wrap all public methods in the Connection context manager,

I actually did this originally (hence the branch name), but felt I was changing things too much. I'm glad you proposed going this direction.

I've updated the PR, incorporating all of your suggestions. Thank you for the detailed explanation!

Let me know if I missed anything or you'd like additional changes.

@Zsailer Zsailer changed the title Handle and log exceptions when writing to database to free up lock Use a context manager for all write actions to prevent indefinite database locks Apr 13, 2024
@Zsailer Zsailer merged commit 3a9d49f into jupyter-server:main Apr 13, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants