django / asgiref

ASGI specification and utilities
https://asgi.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
1.46k stars 207 forks source link

Use `__traceback_hide__` to skip frames in tracebacks #383

Closed bigfootjon closed 1 year ago

bigfootjon commented 1 year ago

This idea came from ongoing work in https://github.com/django/django/pull/16636 to try and hide sensitive variables in an async wrapper for a synchronous function in the auth contrib app in Django.

Basically, Django supports skipping frames that have a truthy __traceback_hide__ value in the frame's locals:

https://github.com/django/django/blob/85b52d22fd2841c34e95b3a80d6f2b668ce2f160/django/views/debug.py#L523-L527

We could take advantage of this pre-existing logic in Django and hide the internals of the asgiref.sync machinery.

Adopting this idea has 2 advantages:

  1. Users don't need to see frames they don't care about, this leans more in to the "just call async_to_sync/sync_to_async functions and don't worry about the rough edges" concept discussed in the README
  2. Hide potentially sensitive variables from tracebacks / error logs.

To elaborate on (2), https://github.com/django/django/pull/16668 was an attempt to hide sensitive variables from async functions and failed, and lots of the discussion in https://github.com/django/django/pull/16636 has been around trying to hide the sensitive variables within asgiref's internal machinery.

Disadvantages that I see:

  1. This is lying to the user. Right now frames internal to Django are shown on tracebacks, but are less eye-catching (see screenshots below) which is a compelling argument against this.
  2. Due to some calls from asgiref to built-in Python code there are a few frame we can't hide, which leads to a disjointed experience (see second set of screenshots).
  3. This is basically only useful in Django (and maybe some testing frameworks?) and otherwise is just noise

The big idea is that from an user's perspective they call async_to_sync or sync_to_async and magic happens and they get the result they want.

Another thought is that we could make this system opt-in:

# asgiref/__init__.py

hide_internals_in_tracebacks = False

# asgiref/sync.py
from asgiref import hide_internals_in_tracebacks

...
def internal_func():
  __traceback_hide__ = hide_internals_in_tracebacks
...

# <user code>.py

import asgiref

asgiref.hide_internals_in_tracebacks = True

from asgiref.sync import sync_to_async

async def foo():
  return await sync_to_async(sync_function)()

Then perhaps Django would opt-in by default, but I'm just designing this on the fly. I'm not committed to this idea but saw an opportunity.

I've prepared some screenshots showing this in-action to get an idea how this would feel for users:

(note, these screenshots were prepared on top of https://github.com/django/asgiref/pull/382, so add one more frame if that PR is not accepted)

Type Current Proposed
sync_to_async current_sync_to_async after_sync_to_async
async_to_sync current_async_to_sync after_async_to_sync
andrewgodwin commented 1 year ago

Makes sense to me!