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

Create one database connection per thread #720

Merged
merged 6 commits into from
Nov 4, 2023

Conversation

tkrabel
Copy link
Contributor

@tkrabel tkrabel commented Oct 29, 2023

Description

Addresses #714 (comment), using a threadlocal to create data base connections.

Fixes #713

Checklist (delete if not relevant):

  • I have added tests that prove my fix is effective or that my feature works
  • I have updated CHANGELOG.md
  • I have made corresponding changes to user documentation for new features
  • I have made corresponding changes to library documentation for API changes

Copy link
Member

@lieryan lieryan left a comment

Choose a reason for hiding this comment

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

Hi @tkrabel, thanks for making this PR, there's one issue with the code currently as they are. Once they are fixed, I think it should be ready to go.

@@ -67,6 +68,7 @@ def filter_package(package: Package) -> bool:


_deprecated_default: bool = object() # type: ignore
thread_local = local()
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, creating the thread_local at the module-level like this would mean that all instances of autoimports would share the same database and database connection, even though they belong to different instances of AutoImport and more importantly that they belong to different Projects:

project_a = Project("/path/to/project/a")
project_b = Project("/path/to/project/b")
ai_a = AutoImport(project_a, ...)
ai_b = AutoImport(project_b, ...)

assert ai_a.connection is not ai_b.connection

That does not look correct to me. I think this can be avoided by moving thread_local to be an instance variable initialized in AutoImport.__init__().

Copy link
Contributor Author

@tkrabel tkrabel Nov 1, 2023

Choose a reason for hiding this comment

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

Thanks for pointing that our! You're right. I moved the thread local to the __init__ and wrote a test that verifies the following passes:

# Test connections
from rope.base.project import Project
from rope.contrib.autoimport.sqlite import AutoImport

ai1 = AutoImport(Project(".."))
ai2 = AutoImport(Project("."))
ai3 = AutoImport(Project("."))

assert ai1.connection is not ai2.connection
assert ai1.connection is not ai3.connection

@tkrabel tkrabel requested a review from lieryan November 1, 2023 07:51
@lieryan
Copy link
Member

lieryan commented Nov 4, 2023

Looks good to me, thanks for the contribution @tkrabel.

@all-contributors add @tkrabel for code contribution

Copy link
Contributor

@lieryan

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

I've put up a pull request to add @tkrabel! 🎉

@lieryan lieryan merged commit 54f7706 into python-rope:master Nov 4, 2023
18 checks passed
@lieryan lieryan added this to the 1.11.0 milestone Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling Autoimport.connection.execute from differnt thread causes error
2 participants