Add a standard entrypoint for WSGI to project template

[Jannis Leidel]

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.

[Jannis Leidel]

Initial patch from ​https://github.com/jezdez/django/compare/feature/wsgi

[Russell Keith-Magee]

Related (possibly duplicate) tickets: #12091, #8927; also of interest -- #12075

[Jannis Leidel]

#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.

[Aymeric Augustin]

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)

[Jannis Leidel]

Latest state

[Jannis Leidel]

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 for settings.py will also have wsgi.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 honor os.environ['DJANGO_SETTINGS_MODULE'].

Fixed.

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.

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 of self._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.

[Jannis Leidel]

Latest changes

[Jacob]

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).

[Jannis Leidel]

@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.

[Jacob]

Milestone 1.4 deleted

[Jannis Leidel]

Latest version with docs

[Carl Meyer]

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.

[Jannis Leidel]

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.

[Carl Meyer]

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 as projectname.wsgi or just wsgi. 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.

[Carl Meyer]

For reference, the ticket tracking the setup_environ sys.path-munging is #15372.

[Carl Meyer]

​https://github.com/django/django/pull/66 is the up-to-date version of this patch. Things still needed:

1. Unit tests for get_wsgi_application and get_internal_wsgi_application.

2. Manual testing of documented setup instructions for gunicorn, uWSGI, and Apache/mod_wsgi.

3. General docs review.

[Preston Holmes]

note, I've submitted a ticket on the uwsgi docs that will have some light merge work with this patch: #17073

[Preston Holmes]

Coverage report of current patch

[Preston Holmes]

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

[Carl Meyer]

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.

[Carl Meyer]

In [17022]:

Fixed #16360 -- Added WSGI entrypoint to startproject layout, and enabled internal servers (runserver and runfcgi) to use an externally-defined WSGI application. Thanks to Armin Ronacher, Jannis Leidel, Alex Gaynor, ptone, and Jacob Kaplan-Moss.