Skip to content
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

D9pouces wsgi.multiprocess 1 #1765

Merged
merged 2 commits into from May 16, 2023
Merged

D9pouces wsgi.multiprocess 1 #1765

merged 2 commits into from May 16, 2023

Conversation

d9pouces
Copy link
Contributor

Description

If you apply Django middlewares on a HttpRequest without "wsgi.multiprocess" in META, an exception is raised by DJT.
This simple patch avoids this bug.

Fixes # (issue)

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

@tim-schilling
Copy link
Contributor

Thank you for the PR. I agree that this change is straightforward and a +1 from me.

Could you please add a test for this, add a line to the changelog and include a description of how this error occurs within Django Channels? All of that will help the toolbar be easier to maintain and more robust moving forward.

@tim-schilling
Copy link
Contributor

@d9pouces could you provide us with the use case that causes this bug?

@tim-schilling
Copy link
Contributor

@matthiask I think should_render_panels should return True if wsgi.multiprocess is not in the headers. The reason being that an asgi application is more likely to be running in an async or similar process that's not going to be compatible.

@matthiask
Copy link
Member

@tim-schilling I checked, PEP-3333 specifies that wsgi.multiprocess must exist when running under WSGI. https://peps.python.org/pep-3333/

You're saying that the better fallback, for example when running under ASGI, would be to render panels right away? I think I'm in agreement.

@tim-schilling
Copy link
Contributor

Yes, that's what I'm saying. A person can still find a way to get this to work via the RENDER_PANELS setting.

I'll push my changes to this branch over lunch today. The other change is including a comment and some tests for this particular method.

d9pouces and others added 2 commits May 15, 2023 20:33
If you apply Django middlewares on a HttpRequest without "wsgi.multiprocess" in META, an exception is raised by DJT. 
This simple patch avoids this bug.
The likely cause of this is that the application is using ASGI since
wsgi.multiprocess is a required key for a WSGI application. Since the
toolbar currently doesn't support async applications, it's pretty likely
that it won't work for this request.

If you're a developer reading this and you think I'm wrong, you can set
the RENDER_PANELS setting to forcibly control this setting.
@tim-schilling tim-schilling merged commit af7d15f into jazzband:main May 16, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants