Bug: Sync `after_exception` handler prints `NoneType: None` when logging stacktrace about litestar HOT 11 CLOSED

The issue is that sync exception handlers run in a different thread. Here's a simple example to reproduce:

import asyncio
import logging


def handler(exc: Exception) -> None:
    logging.getLogger().exception(exc)


async def main() -> None:
    try:
        raise ValueError
    except ValueError as exc:
        await asyncio.get_running_loop().run_in_executor(None, handler, exc)

asyncio.run(main())

The reason for this is that Logger.exception relies on the "current active exception", which specifically refers to the exception being handled in the current thread. Since the newly spawned thread is not actually handling an exception, but simply invoking a function that receives an exception, it can't find a "current active exception". Even if we could set this in the spawned thread, it would probably be a bad idea to do so.

I don't think there's really a way for us to fix this other than to manually pass exception information, including the traceback, around.

I'm inclined to close this as it's a rather edgy edge-case and not an issue with Litestar itself.

Maybe we could add a note in the docs about this.

@tuukkamustonen wdyt?

from litestar.

I mean you wrote:

IMO if we offer to pass a sync or async function, we should always allow sync_to_thread.

I mean if you allow sync_to_thread, ie. for exception handlers or after_exception, how would you define that?

Sorry. What I meant was: We should allow passing sync_to_thread, i.e. make sync_to_thread available as a parameter to pass. Not that we should default it to sync_to_thread=True.

from litestar.

Yeah, and one more question here would be that how did you intend sync_to_thread to be passed (for exception handlers, after exception, et al)?

I think we could a decorator as you suggested + emit a warning if a non-wrapped, sync callable is passed.

from litestar.

This is an interesting case. I'm not 100% sure what's going on, but the behaviour you're seeing is consistent with what you get when you call logger.exception() while not currently handling an exception. This suggests that it's not receiving the proper exception context, but I'm not sure why that is.

from litestar.

Makes sense, thanks for explaining that.

Why does it spawn a new thread, though? As a safeguard to prevent blocking the loop if people end up doing IO there?

This is related to the async exception handlers not being supported. The rationale there was that you want to prevent people from doing IO in the exception handlers. But then, my assumption was that people will just do sync IO then, and block the loop, which is worse than doing async IO in the first place.

Did I understand right that:

• Sync after_exception is spawned into a thread
• Sync exception handlers are not?

Normally, if you don't need to do async IO you might as well just define the function as sync. There's shouldn't be harm/side-effect in that. But here, if after_exception is automatically executed in a thread, the side effect is that you lose a bit RAM, perf, the stacktrace and probably a few more things that you wouldn't expect.

Documentation could address it (though people will bump into it anyway, and only find the docs later).

But I think whatever the approach, harmonization would do good here - establish a rule and follow that, it seems to be a bit mixed.

With dependencies and handler declaration, you have to explicitly pass sync_to_thread=True. If DI was supported also for these callables I guess it would look like...

Litestar(
    after_exception=[Provide(my_after_exception, sync_to_thread=True)],
    exception_handlers={
        Exception: Provide(my_exception_handler, sync_to_thread=True),
    }

But yeah, I think not running the sync in thread by default and just documenting that could work.

(Then, if you want to go fancy, provide a decorator that will run the sync method in thread so people don't have to do inline run_in_executor 🤷🏻‍♂️)

from litestar.

Why does it spawn a new thread, though? As a safeguard to prevent blocking the loop if people end up doing IO there?

Yes

Did I understand right that:

* Sync `after_exception` is spawned into a thread

* Sync exception handlers are not?

Correct

Normally, if you don't need to do async IO you might as well just define the function as sync. There's shouldn't be harm/side-effect in that. But here, if after_exception is automatically executed in a thread, the side effect is that you lose a bit RAM, perf, the stacktrace and probably a few more things that you wouldn't expect.

Also correct, yes

But I think whatever the approach, harmonization would do good here - establish a rule and follow that, it seems to be a bit mixed.

Agreed. There's no consistency here (and in a few other places). These are mostly leftovers from the Starlette integration and we should aim for a path that unifies behaviour here.

With dependencies and handler declaration, you have to explicitly pass sync_to_thread=True. If DI was supported also for these callables I guess it would look like...

Litestar(
    after_exception=[Provide(my_after_exception, sync_to_thread=True)],
    exception_handlers={
        Exception: Provide(my_exception_handler, sync_to_thread=True),
    }

That interface doesn't really make a lot of sense since Provide is a wrapper that allows something ti be injected as a dependency.

But yeah, I think not running the sync in thread by default and just documenting that could work.

I think that's not a good idea since it would deviate from the current behaviour in other places even more. IMO if we offer to pass a sync or async function, we should always allow sync_to_thread.

(Then, if you want to go fancy, provide a decorator that will run the sync method in thread so people don't have to do inline run_in_executor 🤷🏻‍♂️)

That already exists, it's just not part of the public API:

from litestar.utils.sync import ensure_async_callable

@ensure_async_callable
def some_sync_func() -> None:
  ...

we could think about making this available publicly though.

from litestar.

Closing this as per #3071 (comment).

@tuukkamustonen If you want, feel free to open a PR to add the clarification to the docs!

from litestar.

That interface doesn't really make a lot of sense since Provide is a wrapper that allows something ti be injected as a dependency.

That's why I wrote "If DI was supported also for these callables" though (yes, current it's not).

I think that's not a good idea since it would deviate from the current behaviour in other places even more.

Something to consider for 3.0 at least. Harmonization even if it would break something.

IMO if we offer to pass a sync or async function, we should always allow sync_to_thread.

Allow or force? If allow, how would you configure that?

That already exists, it's just not part of the public API
we could think about making this available publicly though

Though if sync_to_thread is/was the default, then there would be no use. If it wasn't, then yes.

from litestar.

Something to consider for 3.0 at least. Harmonization even if it would break something.

Yeah, definitely. We could maybe try to collect those on an issue and then decide what the best way to go about them is.

Allow or force? If allow, how would you configure that?

Not sure I follow. So far it's never forced. sync_to_thread defaults to None, but you get a warning if you leave it unset with a sync callable. IMO it makes sense that it has the same behaviour everywhere.

Though if sync_to_thread is/was the default, then there would be no use. If it wasn't, then yes.

I don't think sync_to_thread=True should be the default if we introduce it to other places. It should behave the same as it does now.

from litestar.

I mean you wrote:

IMO if we offer to pass a sync or async function, we should always allow sync_to_thread.

I mean if you allow sync_to_thread, ie. for exception handlers or after_exception, how would you define that?

Yes, I agree that sync_to_thread should be disabled by default. But when I suggested that earlier you wrote:

I think that's not a good idea since it would deviate from the current behaviour

Thus the confusion. But maybe I misinterpreted something 😄

Harmonization in 3.0 is the key 👍🏻

from litestar.

Yeah, and one more question here would be that how did you intend sync_to_thread to be passed (for exception handlers, after exception, et al)?

from litestar.