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

Thread safe In Memory Connector #1329

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

romainrey
Copy link

@romainrey romainrey commented Feb 16, 2025

Closes #1320

Successful PR Checklist:

PR label(s):

@romainrey romainrey requested a review from a team as a code owner February 16, 2025 19:50
@github-actions github-actions bot added PR type: feature ⭐️ Contains new features PR type: bugfix 🕵️ Contains bug fix and removed PR type: feature ⭐️ Contains new features labels Feb 16, 2025
ewjoachim
ewjoachim previously approved these changes Feb 16, 2025
Copy link
Member

@ewjoachim ewjoachim 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 :) It probably would have take quite a while to find and fix it if it wasn't for dedicated contributors like yourself :)

@ewjoachim ewjoachim dismissed their stale review February 16, 2025 21:22

Hm, it looks like it blocks the tests ^^'

@ewjoachim
Copy link
Member

ewjoachim commented Feb 17, 2025

You have set your PR so as not to enable maintainers to contribute it, so here's the patch that, I think, would fix the CI:

diff --git a/tests/conftest.py b/tests/conftest.py
index f361ea28..2f41d32e 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -258,8 +258,8 @@ def not_opened_app(connector, reset_builtin_task_names) -> app_module.App:


 @pytest.fixture
-def app(not_opened_app: app_module.App):
-    with not_opened_app.open() as app:
+async def app(not_opened_app: app_module.App):
+    async with not_opened_app.open_async() as app:
         yield app

I'll let you apply it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR type: bugfix 🕵️ Contains bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make InMemoryConnector Thread-Safe for Multi-Threaded FastAPI Use Cases
2 participants