Sanic: Request argument as a global variable

[gaojiuli]

I think it's a little long-winded.
in sanic:

@app.route("/")
async def test(request):
    return json({"test": True})

in flask:

@app.route("/")
def hello():
    return "Hello World!"

Since each function takes a request argument, why not set it as a global variable.
Could you tell me the reason why you don't do this，thank you

[aborsu]

Because the request argument changes with every request and they are executed asynchronously. So there would be no way to know which function is accessing it.

[argaen]

@gaojiuli @aborsu Flask is setting it as a variable for the current Thread, a similar approach can be done using the asyncio.Task.current_task() call. Have a look at this repo: https://github.com/Skyscanner/aiotask-context. It's something I coded for storing variables through all the request/response cycle for aiohttp but it's framework agnostic (only depends on asyncio ofc). It's as easy as doing a context.set('request', request_object) at the beginning and then in any part of the code you can do a context.get('request') to retrieve it.

The problem is that the Task is only propagated when await calls are used but it works for the use case we have in our project.

[aborsu]

I actually firmly believe that the global request object in flask is a bad design.
Explicit is better than implicit.

[argaen]

Well global variables is a bad design. Setting variables in a context because its something you need in most of the calls of your functions its not.

Imagine you want to log the request_id to all your log calls. What would you do then, propagate this variable through ALL your code and function calls so you can use it every time you call logger.info? This indeed is really bad practice.

I totally agree with the explicit is better than implicit but principles are there to solve problems not to create them. If we wanted to follow it 100% why would you set a class variable when you can pass the value every time you call a function of class which is more explicit???

[imbolc]

I agree with @aborsu. Pseudo-global request of flask is not only looks surprisingly for new users. It has significant performance issues (not sure it's the same for asyncio.Task.current_task()) and creates additional problems with testing etc.

[pcdinh]

Mutable global variables are unsafe and unacceptable in asynchronous request handling. I don't think we have an option here.

Sanic Server creates a new Request instance per incoming request. It is heap allocated.

[guyskk]

Global variables is hard for testing, I prefer explict request argument, then I may create a request object explict and pass it to view function.

See http://wiki.c2.com/?GlobalVariablesAreBad for more.

[seemethere]

I agree with @aborsu I think going for explicit rather than implicit is the best course of action.

[frnkvieira]

Global variables are a real pain to unit testing.
@argaen if you need to propagate a variable this much there is a clear problem in the app design.
There are other ways to achieve what you want to.

[seemethere]

Going to close this issue citing the thumbs up given to @aborsu

[timka]

@aborsu @argaen You both have point.

And Sanic is not alone here.

For instance, Tornado is also lacking this feature https://github.com/tornadoweb/tornado/issues/2144

As of design, for instance, in Pyramid request is explicitly passed everywhere, but for such cases there's still a documented way of getting current request https://docs.pylonsproject.org/projects/pyramid/en/1.9-branch/narr/threadlocals.html

Also here's PEP-0550 mentioned by @bdarnell https://www.python.org/dev/peps/pep-0550/

[alexpirine]

There are other ways to achieve what you want to.

Please tell, what are these “other ways”? Because I can't find any. And @argaen is right on point for our use case. I want to log the equivalent of Request-Id in every log, because we build microservices and need to be able to find correlated logs easily.

Explicit is better than implicit.

In the general sense, yes, it feels right. But it doesn't help our use case. And a design choice can only be properly judged when we know which problem it tries to solve. A really good design should enable developers to respond to complex business needs, and not limit them to trivialities.

[alexpirine]

For folks trying to find workarounds, PEP 567 might also be a good read.

[ahopkins]

A few things:

1. It is generally better to start a new conversation and reference this old one than to continue on a 3.5 year old thread.
2. In the new documentation I am adding a section specifically on request ID logging. In short, it will likely be something close to what is shown below.
3. There will likely be some more context aware additions to Sanic in the coming year.

import asyncio
import logging
import uuid
from contextvars import ContextVar

from sanic import Sanic
from sanic.log import LOGGING_CONFIG_DEFAULTS
from sanic.response import text


class RequestLogFilter(logging.Filter):
    def filter(self, record):
        request = app.ctxvar.get()
        setattr(record, "request_id", request.ctx.request_id)
        return True


LOGGING_CONFIG = {**LOGGING_CONFIG_DEFAULTS}
LOGGING_CONFIG["filters"] = {"request_log": {"()": RequestLogFilter}}
LOGGING_CONFIG["handlers"]["access_console"]["filters"] = ["request_log"]
LOGGING_CONFIG["formatters"]["access"]["format"] = (
    "%(asctime)s - (%(name)s)[%(levelname)s][req=%(request_id)s][%(host)s]: "
    "%(request)s %(message)s %(status)d %(byte)d"
)

app = Sanic("__BASE__", log_config=LOGGING_CONFIG)

# Specifically not using app.ctx here because that is likely to be used by Sanic in the near future
app.ctxvar = ContextVar("request")


@app.get("/")
async def get1(request):
    await asyncio.sleep(3)
    return text("Done.")


@app.middleware("request")
async def add_req_id(request):
    request.app.ctxvar.set(request)
    request.ctx.request_id = uuid.uuid4()


if __name__ == "__main__":
    app.run(access_log=True, debug=True, port=9999)