Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Comments

🐛 Preserve traceback when an exception is raised in sync dependency withyield#5823

Merged
tiangolo merged 4 commits intofastapi:masterfrom
sombek:master
Dec 3, 2024
Merged

🐛 Preserve traceback when an exception is raised in sync dependency withyield#5823
tiangolo merged 4 commits intofastapi:masterfrom
sombek:master

Conversation

@sombek
Copy link
Contributor

@sombeksombek commentedDec 30, 2022
edited by Kludex
Loading

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

Edit by@Kludex :

MrSalman333, atulenko, suzaku, oinopion, xavier, adriangb, rohitjain, Kalmis, and M1h4n1k reacted with thumbs up emojiofipify, tsak, and Kalmis reacted with heart emojiMrSalman333, xavier, flxdot, and Kalmis reacted with rocket emoji
@github-actions
Copy link
Contributor

📝 Docs preview for commit202133b at:https://63af5d1c07bec913f8d350a9--fastapi.netlify.app

@MohammedAlhajji
Copy link

great workaround. thanks!

@github-actions
Copy link
Contributor

📝 Docs preview for commit411ad02 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

@iudeeniudeen 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 reacted with heart emoji
@sombek
Copy link
ContributorAuthor

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

@yezz123
Copy link
Contributor

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

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

@github-actions
Copy link
Contributor

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

@suzaku
Copy link

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

fromcontextlibimportasynccontextmanagerasasynccontextmanagerfromtypingimportAsyncGenerator,ContextManager,TypeVarimportanyiofromanyioimportCapacityLimiterfromstarlette.concurrencyimportrun_in_threadpoolasrun_in_threadpool# noqa_T=TypeVar("_T")#@asynccontextmanagerasyncdefcontextmanager_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:yieldawaitrun_in_threadpool(cm.__enter__)exceptExceptionase:traceback_=e.__traceback__ok=bool(awaitanyio.to_thread.run_sync(cm.__exit__,type(e),e,None,limiter=exit_limiter            )        )ifnotok:ife.__traceback__isNone:e.__traceback__=traceback_raiseeelse:awaitanyio.to_thread.run_sync(cm.__exit__,None,None,None,limiter=exit_limiter        )# Run this before starting anything FastAPI relatedfromfastapi.dependenciesimportutilsutils.contextmanager_in_threadpool=contextmanager_in_threadpool
biwann13 reacted with thumbs up emojiofipify, sombek, rohitjain, flxdot, Tantas, and M1h4n1k reacted with heart emoji

@github-actions
Copy link
Contributor

📝 Docs preview for commit45317a3 at:https://63ccf7d40fb8da632502fbec--fastapi.netlify.app

@sombek
Copy link
ContributorAuthor

@tiangolo Any idea when can we merge this?

MoritzWeber0, etsvetanov, maciejjaskowski, and Kalmis reacted with thumbs up emoji

@github-actions
Copy link
Contributor

📝 Docs preview for commit11fd1ce at:https://63eb512c20977663c11a0846--fastapi.netlify.app

@etsvetanov
Copy link

Does this PR need additional work or help?

Kalmis reacted with eyes emoji

calebpitan added a commit to framestd/casandra that referenced this pull requestAug 30, 2023
- add async to get correct tracebacks till issue with fastapiand python 3.11 isfixedfastapi/fastapi#5823
@tiangolotiangolo added the featureNew feature or request labelOct 2, 2023
@Kludex
Copy link
Member

Copied code from#8428 (comment), thanks@guillaumegenthial

from typing import AsyncGenerator, ContextManager, TypeVar

import anyio
import anyio.to_thread
Copy link
Member

Choose a reason for hiding this comment

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

nitpick.

@KludexKludex changed the titleKeeping traceback when exception raised in contextmanager_in_threadpoolKeeping traceback when exception raised incontextmanager_in_threadpoolDec 2, 2024
Comment on lines +87 to +88
assert str(last_frame.path) == __file__
assert last_frame.lineno == raise_value_error.__code__.co_firstlineno
Copy link
Member

Choose a reason for hiding this comment

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

If you revert the changes, you can verify that the dependency will not be the last frame (or in any frame, because it will not be present).

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, great, I did this to verify. 🤓

@tiangolo
Copy link
Member

Great job@sombek, thank you! 🚀

And thanks@Kludex for the help, tweaks, and bringing my attention to this on the private channels. 🙇

This will be available in version 0.115.6 released in the next hours. 🚀

@tiangolotiangolo removed the featureNew feature or request labelDec 3, 2024
@tiangolotiangolo added the bugSomething isn't working labelDec 3, 2024
@tiangolotiangolo changed the titleKeeping traceback when exception raised incontextmanager_in_threadpool🐛 Preserve traceback when exception is raised in sync dependency withyieldDec 3, 2024
@tiangolotiangolo merged commit4f81575 intofastapi:masterDec 3, 2024
@tiangolotiangolo changed the title🐛 Preserve traceback when exception is raised in sync dependency withyield🐛 Preserve traceback when an exception is raised in sync dependency withyieldDec 3, 2024
s-rigaud pushed a commit to s-rigaud/fastapi that referenced this pull requestJan 23, 2025
… `yield` (fastapi#5823)Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@tiangolotiangolotiangolo left review comments

@KludexKludexKludex left review comments

+4 more reviewers

@iudeeniudeeniudeen approved these changes

@BilalAlpaslanBilalAlpaslanBilalAlpaslan approved these changes

@yezz123yezz123yezz123 approved these changes

@odiseo0odiseo0odiseo0 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bugSomething isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Traceback stack does not show exact place of error

11 participants

@sombek@MohammedAlhajji@MrSalman333@yezz123@suzaku@etsvetanov@Kludex@tiangolo@iudeen@BilalAlpaslan@odiseo0

[8]ページ先頭

©2009-2026 Movatter.jp