Opened 5 years ago

Closed 5 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: master
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)

16360.1.diff (10.8 KB) - added by Jannis Leidel 5 years ago.
Initial patch from https://github.com/jezdez/django/compare/feature/wsgi
16360.2.diff (17.8 KB) - added by Jannis Leidel 5 years ago.
Latest state
16360.2.2.diff (18.0 KB) - added by Jannis Leidel 5 years ago.
Latest changes
16360.3.diff (51.9 KB) - added by Jannis Leidel 5 years ago.
Latest version with docs
coverage.tar.bz2 (68.4 KB) - added by Preston Holmes 5 years ago.
Coverage report of current patch

Download all attachments as: .zip

Change History (25)

comment:1 Changed 5 years ago by Jannis Leidel

Has patch: set

Changed 5 years ago by Jannis Leidel

Attachment: 16360.1.diff added

comment:2 Changed 5 years ago by Russell Keith-Magee

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

comment:3 Changed 5 years ago by 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.

comment:4 Changed 5 years ago by 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)
Last edited 5 years ago by Aymeric Augustin (previous) (diff)

Changed 5 years ago by Jannis Leidel

Attachment: 16360.2.diff added

Latest state

comment:5 in reply to:  4 Changed 5 years ago by 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.

Changed 5 years ago by Jannis Leidel

Attachment: 16360.2.2.diff added

Latest changes

comment:6 Changed 5 years ago by Chris Streeter

Cc: Chris Streeter added

comment:7 Changed 5 years ago by 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).

comment:8 Changed 5 years ago by 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.

comment:9 Changed 5 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

Changed 5 years ago by Jannis Leidel

Attachment: 16360.3.diff added

Latest version with docs

comment:10 Changed 5 years ago by Carl Meyer

Version: 1.3SVN

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.

comment:11 in reply to:  10 ; Changed 5 years ago by 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.

comment:12 in reply to:  11 Changed 5 years ago by 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.

comment:13 Changed 5 years ago by Carl Meyer

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

comment:14 Changed 5 years ago by Catalin Iacob

Cc: iacobcatalin@… added

comment:15 Changed 5 years ago by tinodb

Cc: tinodb added

comment:16 Changed 5 years ago by 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.
  1. Manual testing of documented setup instructions for gunicorn, uWSGI, and Apache/mod_wsgi.
  1. General docs review.

comment:17 Changed 5 years ago by Preston Holmes

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

Changed 5 years ago by Preston Holmes

Attachment: coverage.tar.bz2 added

Coverage report of current patch

comment:18 Changed 5 years ago by 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

comment:19 Changed 5 years ago by Carl Meyer

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.

comment:20 Changed 5 years ago by Carl Meyer

Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top