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.

Sign up for GitHub

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
Merged

Create one database connection per thread #720

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
592feb4
Create one database connection per thread
tkrabel Oct 29, 2023
97601df
add unit test
tkrabel Oct 29, 2023
681a9b8
add CHANGELOG
tkrabel Oct 29, 2023
bf8d3a3
fix docstring
tkrabel Oct 29, 2023
db187bd
bugfix: every autoimport gets its own connection
tkrabel Nov 1, 2023
dfa7fbd
Merge branch 'master' into make-connection-thread-local
tkrabel Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

- #710, #561 Implement `except*` syntax (@lieryan)
- #711 allow building documentation without having rope module installed (@kloczek)
- #720 create one sqlite3.Connection per thread using a thread local (@tkrabel)

# Release 1.10.0

Expand Down
26 changes: 24 additions & 2 deletions rope/contrib/autoimport/sqlite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from datetime import datetime
from itertools import chain
from pathlib import Path
from threading import local
from typing import Generator, Iterable, Iterator, List, Optional, Set, Tuple

from rope.base import exceptions, libutils, resourceobserver, taskhandle, versioning
Expand Down Expand Up @@ -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 •
edited
Loading

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



class AutoImport:
Expand All @@ -78,9 +80,10 @@ class AutoImport:
"""

connection: sqlite3.Connection
underlined: bool
memory: bool
project: Project
project_package: Package
underlined: bool

def __init__(
self,
Expand Down Expand Up @@ -114,8 +117,9 @@ def __init__(
assert project_package.path is not None
self.project_package = project_package
self.underlined = underlined
self.memory = memory
if memory is _deprecated_default:
memory = True
self.memory = True
warnings.warn(
"The default value for `AutoImport(memory)` argument will "
"change to use an on-disk database by default in the future. "
Expand Down Expand Up @@ -158,6 +162,24 @@ def create_database_connection(
db_path = str(Path(project.ropefolder.real_path) / "autoimport.db")
return sqlite3.connect(db_path)

@property
def connection(self):
"""
Creates a new connection if called from a new thread.

This makes sure AutoImport can be shared across threads.
"""
if not hasattr(thread_local, "connection"):
thread_local.connection = self.create_database_connection(
project=self.project,
memory=self.memory,
)
return thread_local.connection

@connection.setter
def connection(self, value: sqlite3.Connection):
thread_local.connection = value

def _setup_db(self):
models.Metadata.create_table(self.connection)
version_hash = list(
Expand Down
23 changes: 23 additions & 0 deletions ropetest/contrib/autoimport/autoimporttest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from concurrent.futures import ThreadPoolExecutor
from contextlib import closing, contextmanager
from textwrap import dedent
from unittest.mock import ANY, patch
Expand Down Expand Up @@ -85,6 +86,28 @@ def foo():
assert [("from pkg1 import foo", "foo")] == results


def test_multithreading(
autoimport: AutoImport,
project: Project,
pkg1: Folder,
mod1: File,
):
mod1_init = pkg1.get_child("__init__.py")
mod1_init.write(dedent("""\
def foo():
pass
"""))
mod1.write(dedent("""\
foo
"""))
autoimport = AutoImport(project, memory=False)
autoimport.generate_cache([mod1_init])

tp = ThreadPoolExecutor(1)
results = tp.submit(autoimport.search, "foo", True).result()
assert [("from pkg1 import foo", "foo")] == results


@contextmanager
def assert_database_is_reset(conn):
conn.execute("ALTER TABLE names ADD COLUMN deprecated_column")
Expand Down