Opened 13 years ago
Closed 13 years ago
#16360 closed New feature (fixed)
Add a standard entrypoint for WSGI to project template
Reported by: | Jannis Leidel | Owned by: | nobody |
---|---|---|---|
Component: | Core (Other) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Armin Ronacher, Chris Streeter, iacobcatalin@…, tinodb | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
As discussed at DjangoCon Europe and EuroPython, this is about adding a standard API to Django for hooking WSGI frameworks in and extending Django's own WSGI application with 3rd party WSGI middlewares.
Attachments (5)
Change History (25)
comment:1 by , 13 years ago
Has patch: | set |
---|
by , 13 years ago
Attachment: | 16360.1.diff added |
---|
comment:2 by , 13 years ago
comment:3 by , 13 years ago
#12091 is definitely out of the scope of this ticket. The attached patch only extends the already existing support for WSGI to be easier to combine with other WSGI components.
#8927 and #12075 cover other parts of the stack that wouldn't be touched by this ticket in any case. In other words, those tickets can happen even if this ticket is applied.
follow-up: 5 comment:4 by , 13 years ago
This review is based on code inspection of the branch at https://github.com/jezdez/django/compare/feature/wsgi in its current state.
django/conf/global_settings.py
Isn't the default 'django.core.handlers.wsgi.application'
rather than 'django.core.handlers.wsgi.WSGIHandler'
?
django/conf/project_template/settings.py
No other setting in this file is commented out. In this regard, WSGI_APPLICATION
looks inconsistent. New Django projects using this template for settings.py
will also have wsgi.py
, so it would be logical and consistent to uncomment this line.
I wondered if this setting is actually useful. I concluded it is, because it makes the location of the WSGI application explicit, and because people may run different websites one the same codebase with different WSGI entrypoints. The argument is less compelling than for ROOT_URLCONF
but I guess it could happen.
I'm not sure if "for using" is correct, I'd write "to use" instead.
django/conf/project_template/wsgi.py
I think the docstring should be moved to the documentation — as far as I can tell, the docs aren't written yet. (Do you want help writing them?)
(minor) The "Make sure..." sentence isn't necessary if WSGI_APPLICATION
is set by default (see above).
The comment about using '{{ project_name }}.settings' is misleading, because setup_environ
will honor os.environ['DJANGO_SETTINGS_MODULE']
.
Also, the code will fail if there is no settings
module on PYTHONPATH
. Isn't it possible to make it work, provided DJANGO_SETTINGS_MODULE
is set? Based on https://docs.djangoproject.com/en/dev/howto/deployment/modwsgi/ it looks like it's possible.
django/contrib/staticfiles/management/commands/runserver.py
(minor) I think:
enabled = (settings.DEBUG and use_static_handler or (use_static_handler and insecure_serving))
would be more readable as:
enabled = use_static_handler and (settings.DEBUG or insecure_serving)
django/core/handlers/wsgi.py
Line 322 looks wrong: it returns self._instance
instead of self._instance(environ, start_response)
.
I suppose you first check if self._instance is not None
to avoid the performance hit of taking the lock; this optimization should be explained in a quick comment.
I think the whole function would be easier to understand like this:
# Non-atomic check, avoids taking a lock at each call of this method if self._instance is None: with self._instance_lock: # Atomic check, prevents concurrent initialization of self._instance if self._instance is None: # ... initialize self._instance ... return self._instance(environ, start_response)
comment:5 by , 13 years ago
Replying to aaugustin:
This review is based on code inspection of the branch at https://github.com/jezdez/django/compare/feature/wsgi in its current state.
django/conf/global_settings.py
Isn't the default
'django.core.handlers.wsgi.application'
rather than'django.core.handlers.wsgi.WSGIHandler'
?
Fixed.
django/conf/project_template/settings.py
No other setting in this file is commented out. In this regard,
WSGI_APPLICATION
looks inconsistent. New Django projects using this template forsettings.py
will also havewsgi.py
, so it would be logical and consistent to uncomment this line.
Fixed.
I wondered if this setting is actually useful. I concluded it is, because it makes the location of the WSGI application explicit, and because people may run different websites one the same codebase with different WSGI entrypoints. The argument is less compelling than for
ROOT_URLCONF
but I guess it could happen.
The important message this setting should clarify is that Django is capable of housing WSGI applications that are comprised of more than the standard WSGI handler.
django/conf/project_template/wsgi.py
I think the docstring should be moved to the documentation — as far as I can tell, the docs aren't written yet. (Do you want help writing them?)
Yeah, the docs haven't been covered yet since I wasn't sure where to put it. I was thinking about a general WSGI section which links to mod_wsgi and uWSGI deployment docs, too.
(minor) The "Make sure..." sentence isn't necessary if
WSGI_APPLICATION
is set by default (see above).
Fixed.
The comment about using '{{ project_name }}.settings' is misleading, because
setup_environ
will honoros.environ['DJANGO_SETTINGS_MODULE']
.
Fixed.
Also, the code will fail if there is no
settings
module onPYTHONPATH
. Isn't it possible to make it work, providedDJANGO_SETTINGS_MODULE
is set? Based on https://docs.djangoproject.com/en/dev/howto/deployment/modwsgi/ it looks like it's possible.
I've updated the code to properly setup the environment (with the setup_settings) command.
django/contrib/staticfiles/management/commands/runserver.py
(minor) I think:
enabled = (settings.DEBUG and use_static_handler or (use_static_handler and insecure_serving))would be more readable as:
enabled = use_static_handler and (settings.DEBUG or insecure_serving)
Fixed.
django/core/handlers/wsgi.py
Line 322 looks wrong: it returns
self._instance
instead ofself._instance(environ, start_response)
.
Fixed.
I suppose you first check
if self._instance is not None
to avoid the performance hit of taking the lock; this optimization should be explained in a quick comment.
I think the whole function would be easier to understand like this:
# Non-atomic check, avoids taking a lock at each call of this method if self._instance is None: with self._instance_lock: # Atomic check, prevents concurrent initialization of self._instance if self._instance is None: # ... initialize self._instance ... return self._instance(environ, start_response)
Fixed.
comment:6 by , 13 years ago
Cc: | added |
---|
comment:7 by , 13 years ago
We've closed #7930 as a duplicate of this one, so we should double-check that this change would fix that bug (I'm not sure it does, but it should).
comment:8 by , 13 years ago
@jacob I'm not convinced #7930 is a duplicate of this ticket as it's about a specific bug in how we handle the script prefix, not about adding a new WSGI entrypoint. Re-opening.
follow-up: 11 comment:10 by , 13 years ago
Version: | 1.3 → SVN |
---|
This generally looks great to me. I'm making some minor doc tweaks to the patch (and merged up to current trunk) here: https://github.com/carljm/django/compare/master...feature/wsgi
I'd also like to try a more explicit approach to settings, using a dotted path to the settings module in wsgi.py
rather than __name__
and removing the implicit settings-module-finding code in setup_settings. I'd like to avoid adding any more implicit behavior to settings-handling if possible.
follow-up: 12 comment:11 by , 13 years ago
Replying to carljm:
This generally looks great to me. I'm making some minor doc tweaks to the patch (and merged up to current trunk) here: https://github.com/carljm/django/compare/master...feature/wsgi
Awesome, thanks!
I'd also like to try a more explicit approach to settings, using a dotted path to the settings module in
wsgi.py
rather than__name__
The big advantage for __name__
is the fact that it allows the wsgi module to understand how the deployment server imports the file, whether as projectname.wsgi
or just wsgi
. I believe that's not implicit or magical, just using a Python feature.
and removing the implicit settings-module-finding code in setup_settings. I'd like to avoid adding any more implicit behavior to settings-handling if possible.
That's also sensible to make sure the wsgi file doesn't fall in the trap of having to add the parent dir of a project to the sys.path manually. I think that's a reasonable thing to do, a magic with limited scope, so to say.
comment:12 by , 13 years ago
Replying to jezdez:
I'd also like to try a more explicit approach to settings, using a dotted path to the settings module in
wsgi.py
rather than__name__
The big advantage for
__name__
is the fact that it allows the wsgi module to understand how the deployment server imports the file, whether asprojectname.wsgi
or justwsgi
. I believe that's not implicit or magical, just using a Python feature.
Using name isn't adding implicit behavior, that's just a Python feature as you say. But automatically finding the settings module if its "next" to wsgi.py, and otherwise requiring DJANGO_SETTINGS_MODULE to be set, is adding implicit behavior (roughly parallel implicit behavior to what manage.py already does, but nonetheless spreading it beyond manage.py).
Anyhow, after our IRC conversation I understand the reasons for this, and they are understandable, even good reasons. Thanks to the sys.path-munging in setup_environ, we've long since set up the expectation that we'll set up sys.path for people (and allow them to import stuff as either myproject.* or just *). This patch just expands that already-longstanding magic to cover not only manage.py and runserver but all WSGI deployments, so people will hit fewer hurdles going from development to production, which is certainly a good goal.
In the general theme of making Django play nicer as a Python library, though, I think the sys.path-munging in setup_environ is a real problem, and I know other people have identified it as one. It causes real-world issues, like the double-imports issue from overlapping sys.path, and the problem where people commit their project as top-level in a VCS repo and then it breaks when checked out under a different name, or a not-valid-Python-module name. So I would love to take this opportunity to see if we can fix that in a backwards-compatible way before we expand its reach beyond manage.py/runserver.
I have some ideas for how we might do that; won't go into it here but I'll post to the mailing list soon with some working code.
If I don't get to that in time, though, I don't want to hold this up for it. I think we can still fix it after this has gone in, though it'll be harder and involve more deprecations.
comment:13 by , 13 years ago
For reference, the ticket tracking the setup_environ sys.path-munging is #15372.
comment:14 by , 13 years ago
Cc: | added |
---|
comment:15 by , 13 years ago
Cc: | added |
---|
comment:16 by , 13 years ago
https://github.com/django/django/pull/66 is the up-to-date version of this patch. Things still needed:
- Unit tests for get_wsgi_application and get_internal_wsgi_application.
- Manual testing of documented setup instructions for gunicorn, uWSGI, and Apache/mod_wsgi.
- General docs review.
comment:17 by , 13 years ago
note, I've submitted a ticket on the uwsgi docs that will have some light merge work with this patch: #17073
comment:18 by , 13 years ago
I've attached is a coverage report for the patch at this point in time, the report is generated by https://gist.github.com/1302777
Lines that are added or modified by the patch have a green highlight on line number.
looks like current is well covered
comment:19 by , 13 years ago
Needs documentation: | unset |
---|
This now has tests, several people have reviewed the docs, and the instructions for all three WSGI servers have been tested and work. I think it's ready and will commit soon, though I wouldn't mind having someone do a final once-over first for anything I may have missed.
Initial patch from https://github.com/jezdez/django/compare/feature/wsgi