-
-
Notifications
You must be signed in to change notification settings - Fork 799
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lifespan support #1216
Comments
How do you see it being implemented? I hadn't really considered doing stuff in Channels with it. |
@andrewgodwin to be honest, I am not entirely sure what all this would entail. I am mostly concerned because ASGI servers like Uvicorn are starting to expect this support and, consequently, break with Django Channels. I might be interested in using the feature to close a socket used by multiple clients after a specific time, and then expire all their tokens. |
Ah yes, we should at least make Channels just ignore |
I think that would be a good place to start. Maybe allow the setting to ignore to be overridden in case the user wants to extend channels to handle lifecycle? |
I think this issue is related to my problem. In my consumer, I create a
|
OK, so I'm working on Lifespan support in Daphne right now for django/daphne#264. That'll be this week/next week/some-point-this-summer depending on time available and how it goes. At that point I'll roll a new Daphne release and cut back to Channels for improvements here. Chunked body handling, proper AGSI 3 support, and, yes, Lifespan are on the list for that. (Any contributions towards that list on the channels end are gratefully received.) |
What sort of complexity/challenge do you expect for the channels support? Are some pieces easy enough for first time contributors? |
Hi @jheld. I don't suppose it'd be all that tricky... The app needs to respond to Lifespan messages... (Receive a I'd be super-happy to advise if anyone wants to try putting something together. |
@carltongibson Fair! Yeah, I'm interested to help implement this (or parts). Sounds fun and looks like it wants to provide sensible ignore (if not using), and those hooks when it does. Would we need an async version of this class, too, or are the hooks intended to be synchronous all the time? If both, perhaps getting the sync version first, and another PR for async (if there is concern around level of effort or complexity). |
I imagine lifespan events to only need synchronous versions. The server is required to wait for the response before continuing (to serve requests or shutdown as appropriate) so it is effectively blocked anyway... |
Cool! Got it. Other than underlying daphne support getting merged (uvicorn would need it, too, separately, I would expect if it doesn't already have it), I could look into this next week, getting a WIP/PoC up. |
Awesome, thanks. Uvicorn already has support. I'm just trying to get the spare few hours together for the Daphne version. (Soon™ 🙂) If you can pull in a PR here that would be 🎉 |
In case it is of interest here is the output I get when I start a channels project with hypercorn:
Here is the code from hypercorn:
As you can see the traceback gets printed to the console with log.exception(), although it is handled so the program continues to execute. |
I thought lifespan messages were not supposed to crash the server? Or maybe it's just the startup or lifespan scope.. The docs are explicit about those but not others, so perhaps that's correct then? |
@jheld I think it's just on startup, after that traceback is printed the server seems to work fine. |
Just to be clear, Hypercorn isn't crashing it is just printing the exception. I'm looking forward to lifespan support in Channels. |
If the startup message raises an error the server must continue and stop sending further lifespan messages. (So Hypercorn looks like it's doing the right thing.) |
Hello, what's the current progress on this ticket? A project I'm working on (proprietary) can really use this feature and we're considering contributing (well, it's just me) if it isn't going to be available soon. |
@chtseac Super. See django/daphne#264, which would have Daphne send the lifespan events. I began playing with that, but didn't have the time to finish it. If you wanted to contribute there, that would be amazing. Currently, I want to first resolve the family of issues around |
@carltongibson can you point to what bugs exist on |
@jheld Super! Glad to have you onboard. See the issues and PRs I assigned to myself: Issues, PRs. They're all in a cluster. (But not all exactly the same issue...) See #1334 (review) - As per that, we shouldn't catch the exception and call stop consumer. (We should let it bubble up, and have Daphne close the consumer on sending the error response.) Have a dig in. Let me know on the relevant issue what doesn't make sense. I'm very happy to advise/support: I've been on my own here and haven't had the bandwidth to finish those issues off. |
:( |
Any new development on this issue? It is really killing us on our sentry quota. |
@pe82 I do have an open PR for this support. It's untested, but if you're willing to give it a try (possibly forking & rebasing from |
Unfortunately I don't have a local set up to test this, I looked at the code though... seems valid. Out of curiosity, are there no official reviewers for this project? Is it abandoned? it seems the PR was submitted very long time ago. |
No, not abandoned, but just me, and Lifespan isn't high on the priority list really. All you need is an ASGI middleware. There's no real reason is needs to be in channels itself. |
@pe82 Here is a shim that worked for me when I tested it last year:
Then in my |
@pe82 Just out of interest, how's this? If I recall the spec correctly, lifespan messages are meant to be sent once at startup, and if the application doesn't support it, no more lifespan messages are meant to be sent. So... 🤔 I'd expect at most one error message to sentry per application restart? |
If this kills your Sentry quota it's more appropriate to filter out the error in Sentry or raise an issue with Sentry. You can paste the string |
Closing as per #1330 — there's not much value in just having an empty stub consumer. |
@carltongibson there may not be much value in the code, but from a developer perspective this would make it faster/easier to get setup. From my understanding, the current default setup generates noisy errors that we expect the developer to research and solve by ignoring lifespan messages. I'd like to see it part of channels to make it another sensible default as a part of the library and ecosystem. If you disagree, would you be open to a PR adding some type of notice to the Troubleshooting and Consumers portions of the docs? |
Hey @tim-schilling — I'm not quite sure what you're proposing. Only with Alternatively, you can implement an ASGI middleware to respond to Lifespan events. |
Fair enough.
I just saw that. D'oh. |
Lifespan is a new addition to the ASGI specification that provides events pertaining to the main event loop. You can find its documentation here: https://asgi.readthedocs.io/en/latest/specs/lifespan.html
Is there any interest in implementing this in Django Channels?
The text was updated successfully, but these errors were encountered: