Opened 4 years ago

Closed 3 years ago

#16360 closed New feature (fixed)

Add a standard entrypoint for WSGI to project template

Reported by: jezdez Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: mitsuhiko, 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 jezdez 4 years ago.
Initial patch from https://github.com/jezdez/django/compare/feature/wsgi
16360.2.diff (17.8 KB) - added by jezdez 4 years ago.
Latest state
16360.2.2.diff (18.0 KB) - added by jezdez 4 years ago.
Latest changes
16360.3.diff (51.9 KB) - added by jezdez 3 years ago.
Latest version with docs
coverage.tar.bz2 (68.4 KB) - added by ptone 3 years ago.
Coverage report of current patch

Download all attachments as: .zip

Change History (25)

comment:1 Changed 4 years ago by jezdez

  • Has patch set

Changed 4 years ago by jezdez

comment:2 Changed 4 years ago by russellm

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

comment:3 Changed 4 years ago by jezdez

#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 follow-up: Changed 4 years ago by 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'?

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 more 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)
Version 0, edited 4 years ago by aaugustin (next)

Changed 4 years ago by jezdez

Latest state

comment:5 in reply to: ↑ 4 Changed 4 years ago by jezdez

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 4 years ago by jezdez

Latest changes

comment:6 Changed 4 years ago by streeter

  • Cc streeter added

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

@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 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

Changed 3 years ago by jezdez

Latest version with docs

comment:10 follow-up: Changed 3 years ago by carljm

  • Version changed from 1.3 to 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.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by jezdez

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 3 years ago by carljm

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 3 years ago by carljm

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

comment:14 Changed 3 years ago by cataliniacob

  • Cc iacobcatalin@… added

comment:15 Changed 3 years ago by tinodb

  • Cc tinodb added

comment:16 Changed 3 years ago by carljm

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 3 years ago by ptone

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

Changed 3 years ago by ptone

Coverage report of current patch

comment:18 Changed 3 years ago by ptone

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 3 years ago by carljm

  • 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 3 years ago by carljm

  • Resolution set to fixed
  • Status changed from new to closed

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