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

Keeping traceback when exception raised in contextmanager_in_threadpool #5823

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sombek
Copy link

@sombek sombek commented Dec 30, 2022

There is a missing of traceback when an exception raised in contextmanager_in_threadpool
Happens on Python 3.11

I noticed the exception.traceback gets flushed after exiting the context manager.

There is an issue is opened #5740

@github-actions
Copy link
Contributor

📝 Docs preview for commit 202133b at: https://63af5d1c07bec913f8d350a9--fastapi.netlify.app

@MohammedAlhajji
Copy link

great workaround. thanks!

@github-actions
Copy link
Contributor

📝 Docs preview for commit 411ad02 at: https://63af6350238dde190a34aafc--fastapi.netlify.app

@MrSalman333
Copy link

Nice work!

We didn't upgrade to 3.11 because of this.
Thankss

Copy link
Contributor

@iudeen iudeen left a comment

Choose a reason for hiding this comment

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

Looks neat! And thanks for the tests as well!

@sombek
Copy link
Author

sombek commented Jan 3, 2023

@iudeen @yezz123 Usually, how long it takes you guys to merge a PR?

@yezz123
Copy link
Contributor

yezz123 commented Jan 3, 2023

@iudeen @yezz123 Usually, how long it takes you guys to merge a PR?

Mainly @tiangolo will review it after adding a flag of awaiting review so just wait it's will be reviewed asap

@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2023

📝 Docs preview for commit efd395c at: https://63bab4ceb11ed37e88ef871e--fastapi.netlify.app

@suzaku
Copy link

suzaku commented Jan 11, 2023

I patched your change with the following code and now the traceback's back. Thanks.

from contextlib import asynccontextmanager as asynccontextmanager
from typing import AsyncGenerator, ContextManager, TypeVar

import anyio
from anyio import CapacityLimiter
from starlette.concurrency import run_in_threadpool as run_in_threadpool  # noqa

_T = TypeVar("_T")

# 
@asynccontextmanager
async def contextmanager_in_threadpool(
    cm: ContextManager[_T],
) -> AsyncGenerator[_T, None]:
    # blocking __exit__ from running waiting on a free thread
    # can create race conditions/deadlocks if the context manager itself
    # has it's own internal pool (e.g. a database connection pool)
    # to avoid this we let __exit__ run without a capacity limit
    # since we're creating a new limiter for each call, any non-zero limit
    # works (1 is arbitrary)
    exit_limiter = CapacityLimiter(1)
    try:
        yield await run_in_threadpool(cm.__enter__)
    except Exception as e:
        traceback_ = e.__traceback__
        ok = bool(
            await anyio.to_thread.run_sync(
                cm.__exit__, type(e), e, None, limiter=exit_limiter
            )
        )
        if not ok:
            if e.__traceback__ is None:
                e.__traceback__ = traceback_
            raise e
    else:
        await anyio.to_thread.run_sync(
            cm.__exit__, None, None, None, limiter=exit_limiter
        )

# Run this before starting anything FastAPI related
from fastapi.dependencies import utils
utils.contextmanager_in_threadpool = contextmanager_in_threadpool

@github-actions
Copy link
Contributor

📝 Docs preview for commit 45317a3 at: https://63ccf7d40fb8da632502fbec--fastapi.netlify.app

@sombek
Copy link
Author

sombek commented Feb 2, 2023

@tiangolo Any idea when can we merge this?

@github-actions
Copy link
Contributor

📝 Docs preview for commit 11fd1ce at: https://63eb512c20977663c11a0846--fastapi.netlify.app

@etsvetanov
Copy link

Does this PR need additional work or help?

@tiangolo tiangolo added the feature New feature or request label Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants