Refactor redirect middleware using contextvars

[florimondmanca]

While #325, and having #305 (comment) in mind, I noticed that the history was a middleware instance variable on RedirectMiddleware, and that surely it could be refactored to use context variables instead. So, here we are! I think the result is quite pleasing.

The docs for contextvars, if need be: contextvars.

@yeraydiazdiaz The second commit here illustrates what I suggested in #305 (comment):

I think we should be able to convert all utility methods into regular functions, and end up with the typical init()/call() pair of methods, with the logic in call delegating as much as possible to the helper functions.

[florimondmanca]

On 3.6, I used the aiocontextvars backport which includes asyncio support (while the contextvars backport does not (yet)).

We may be running into issues when extending this to add trio support, though. Trio has built-in support for context variables (see task-local storage), and it installs the contextvars backport on < 3.7. This means that users that have trio installed and try to use the asyncio backend (e.g. with the sync Client) will run into issues because they'll end up using the contextvars backport which lacks asyncio support.

One solution I see is to build a custom ContextVar class that holds both versions, and relies on sniffio to detect the async environment and defer calls to the correct implementation. This should hold so long as the main contextvars backport lacks asyncio support (which is planned to not be the case forever… except if it ends up being so?).

I guess we can address this problem in due time, but it's good to know that if we go down the path of context variable we'll need to add some compatibility glue to make sure it works seamlessly on asyncio and trio across all supported Python versions.

Thoughts?

[florimondmanca]

A non-asyncio-specific refactor could look something like this:

import sys
import typing
import sniffio

# Also available on Python 3.6+ via aiocontextvars.
import contextvars as _contextvars

try:
    import aiocontextvars as _aiocontextvars
except ImportError:  # pragma: no cover
    _aiocontextvars = None  # type: ignore

T = typing.TypeVar("T")


class ContextVar(typing.Generic[T]):
    """Stripped-down version of contextvars.ContextVar.

    Compatible with asyncio / Python 3.6.
    """

    def __init__(self, name: str) -> None:
        self._default_var: _contextvars.ContextVar[T] = _contextvars.ContextVar(name)
        self._asyncio_var = (
            _aiocontextvars.ContextVar(name) if sys.version_info < (3, 7) else None
        )

    def _get_variable(self) -> typing.Any:
        if (
            sniffio.current_async_library() == "asyncio"
            and self._asyncio_var is not None
        ):
            return self._asyncio_var
        return self._default_var

    def get(self) -> T:
        variable = self._get_variable()
        return variable.get()

    def set(self, value: T) -> None:
        variable = self._get_variable()
        variable.set(value)

I don't think we should include it here for the sake of YAGNI, but I think this should work just fine on trio, or at least it shouldn't be a major blocker. Like I said though, we can dig further in due time.

[tomchristie]

Hold up there. 😃
We don't want to be using task local or thread local state in any of these cases. If we think we need task local or thread local state for something here, then we're probably doing it wrong.

What we do want is plain old local variables (passed around if needed), rather than state that's being persisted on the class.

[florimondmanca]

@tomchristie I hear you. I let this PR up to let the idea decay, and in retrospect I can see how context variables are definitely not what we need to use in this particular situation.

I guess what we want is for RedirectMiddleware.call to define the recursive function and return a first call to it, with history defined as a local variable enclosed within the recursive function, right?

Anyway I’ll close this and if the above sounds about right I can tackle that refactor instead.