Optional uvloop use

[prryplatypus]

Resolves #2223.

[prryplatypus]

I've made it NO_UVLOOP for consistency, but it's not super user friendly when we do if not self.config.NO_UVLOOP:. I'd suggest changing it to USE_UVLOOP (both in the install and in config), but make it default to True instead.

[ahopkins]

That's a breaking change. Not ideal but I think keeping it negative might be the way to go.

[prryplatypus]

So we're not willing to do this? 🙈

import os

USE_UVLOOP = os.getenv('USE_UVLOOP', True)
no_uvloop = os.getenv('NO_UVLOOP', None)

if no_uvloop is not None:
    # Raise DeprecationWarning
    USE_UVLOOP = no_uvloop

[ahopkins]

Why do they need to be connected? The SANIC_NO_UVLOOP variable is meant to be an install time switch. Something that itself needs to be fixed (if possible). But, if it is set, then theoretically, you should not have uvloop installed, and it should not matter.

I think we can just go forward with Config.USE_UVLOOP and default to that. If there is a conflict (``USE_UVLOOP == TrueandNO_UVLOOP == True`), then this is a situation that we could raise a warning. But, this should only matter if uvloop is installed. If it is not installed, and you have `USE_UVLOOP == True`, then that is another scenario we need to raise a startup warning.

[prryplatypus]

Alright, that sounds good to me!

[prryplatypus]

I uhhh... may need some help with these tests. I'm not too used to writing tests, and I think this is not one of the easiest things to write them for either 😅. Could anyone lend me a hand, please?

EDIT: Just realised that this method will not work for those who update their config after initialising the app. I'll have to give this another try soon.

[prryplatypus]

@ahopkins, could you confirm if I would have to call the _configure_event_loop method from __call__ too? I'm not too sure how ASGI works.

[prryplatypus]

It should work as it currently is for most usecases, but I'm unsure about what would happen if there was several Sanic apps running some with the USE_UVLOOP config set to True and others with it set to False. Does anyone here have an idea of how we could solve this "edge-case"?

EDIT: After some discussion on Discord we figured it'd be best to default all to use uvloop unless ALL of them are False.

[prryplatypus]

Things worth noting:

• If running more than one app, do we want to raise a warning if their configs are different as we can't have some apps work with one loop policy and some apps work with another?
• When running the app with create_server, do we maybe want to check if the config loop policy matches the currently running event loop and raise a warning only if it doesn't match? For example, if config.USE_UVLOOP is explicitly set to True but asyncio.get_event_loop() doesn't return a uvloop.Loop instance.
• When running Sanic with Gunicorn (worker.py), uvloop is always configured to be used. Do we want to add a warning about this or just put it in the docs?

[ahopkins]

This looks great 🎉

Nice work on this PR 💪

[ahopkins]

• If running more than one app, do we want to raise a warning if their configs are different as we can't have some apps work with one loop policy and some apps work with another?

Hmm... Yes, but if one is set to _default then I suppose it does not matter. The issue would be if one is True and one False. Correct?

• When running the app with create_server, do we maybe want to check if the config loop policy matches the currently running event loop and raise a warning only if it doesn't match? For example, if config.USE_UVLOOP is explicitly set to True but asyncio.get_event_loop() doesn't return a uvloop.Loop instance.

create_server is a pretty DIY. If anything, I would say that we could do a check in the AsyncioServer.startup to see if it is not _default, in which case we throw a warning. But no need to check the types.

When running Sanic with Gunicorn (worker.py), uvloop is always configured to be used. Do we want to add a warning about this or just put it in the docs?

Docs.

[prryplatypus]

Hmm... Yes, but if one is set to _default then I suppose it does not matter. The issue would be if one is True and one False. Correct?

Correct! Or if one is _default and the other one if False and the _default one is started first (as it will then override the event loop config).

create_server is a pretty DIY. If anything, I would say that we could do a check in the AsyncioServer.startup to see if it is not _default, in which case we throw a warning. But no need to check the types.

We already do this inside the create_server function, so I guess that works then!

Docs

💯

[prryplatypus]

This is as far as I can get. I've tried adding the following test for the asgi warning, and it works when running it individually, but not when running it with all the other tests in tests/test_asgi.py, and I can't figure out why. Any help is welcome. That should be the last thing left to do (also replacing vXX.X with some actual version for deprecation).

def test_non_default_uvloop_config_raises_warning(app):
    app.config.USE_UVLOOP = True

    class CustomServer(uvicorn.Server):
        def install_signal_handlers(self):
            pass

    config = uvicorn.Config(app=app, loop="asyncio", limit_max_requests=0)
    server = CustomServer(config=config)

    with pytest.warns(UserWarning) as records:
        server.run()

    all_tasks = asyncio.all_tasks(asyncio.get_event_loop())
    for task in all_tasks:
        task.cancel()

    msg = ""
    for record in records:
        _msg = str(record.message)
        if _msg.startswith("You have set the USE_UVLOOP configuration"):
            msg = _msg
            break

    assert msg == (
        "You have set the USE_UVLOOP configuration option, but Sanic "
        "cannot control the event loop when running in ASGI mode."
        "This option will be ignored."
    )