Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Ticket 18855: persist socket across runserver reloads #893

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants

See discussion of this ticket on the bug tracker.

Creating a new pull request as advised in the ticket after adding documentation of the feature.

The testing part of this ticket is especially tricky. The only way I see to test the actual functionality would be to fork a new process and run "runserver" within the process (if you have other ideas, I'd love to hear them) and then interact with the new process via the opened port. Then there is the case of the race condition that this effectively tries to solve.

Given the wide use of runserver, I think that it's relatively safe to not add tests (given the difficultly of doing so). If you have any ideas on how to make this test-able, I'd definitely take suggestions and attempt to add tests for them.

@unaizalakain attempted to address your comments. What's the next step?

Contributor

unaizalakain commented Nov 26, 2013

LGTM. Squash all your commits together (with git rebase -i) so that the PR consists of a single commit. I'll mark it RFC in the ticket and eventually a core dev will come up and put it into 1.7 ;)

Alright, squashed everything into a single commit.

Thanks so much for your help!

Contributor

Bouke commented Dec 14, 2013

Nice, this bug has annoyed me for a long time. It would be great to have it merged into 1.7!

@apollo13 apollo13 commented on the diff Dec 15, 2013

django/core/management/commands/runserver.py
@@ -81,6 +83,16 @@ def run(self, *args, **options):
"""
use_reloader = options.get('use_reloader')
+ # test if socket.fromfd is supported on this platform as it's needed
+ # later for this code to work
+ if options.get('use_persist_sock') and getattr(socket, 'fromfd', False):
+ address_family = socket.AF_INET
+ if self.use_ipv6:
+ address_family = socket.AF_INET6
+ if not os.environ.get('DJANGO_SERVER_FD'):
@apollo13

apollo13 Dec 15, 2013

Owner

If it is already in the env, can we be sure that we are able to reuse the socket? Does the following code work in pyhton 3.4? I think sockets are not inherited by child processes there. (On the run, but should get investigated)

@dlamotte

dlamotte Dec 15, 2013

http://www.python.org/dev/peps/pep-0433/#close-file-descriptors-after-fork

I believe in Python 3.3+, the CLOSE_ON_EXEC flag is set by default. However, since we're only forking without exec (IIRC), this file descriptor will stay open. It looks as though they're maybe planning on making some way to easily close file descriptors with this flag using an atfork module, but for now, it should work (although admittedly, I haven't tested it).

@apollo13

apollo13 Dec 15, 2013

Owner

Another possibly stupid question (I really have no idea of this stuff :)): How does the socket/fd stay valid if the sock variable is local here?

@dlamotte

dlamotte Dec 15, 2013

Not a stupid question :)

You actually scared me a bit there (thought I overlooked this). Python would clean up that variable and close the socket once we return from that function, but since the autoreload.main or inner_run loop forever, the variable never gets cleaned up. Which means that the parent program still references the socket and the fd remains valid so that the child program can refer to the fd and open it.

dlamotte commented Feb 7, 2014

Rebased on top of master.

Owner

timgraham commented May 16, 2014

Has anyone tested the patch on Python 3.4 as noted on the ticket?

Contributor

unaizalakain commented May 17, 2014

Besides the noted changes, patch works like a charm on Python 3.4.

@unaizalakain please take a look at the latest patch. Wasn't sure what you meant by using the six module. I ended up just doing an import inside a try: ... except ImportError: ... to handle the SocketServer to socketserver rename in python 3.x.

I replaced WSGIServerException with socket.error.

I also noticed (after testing) that python 3.4 requires socket be set_inheritable: https://docs.python.org/dev/library/socket.html#socket.socket.set_inheritable So if this method exists on the socket, I have the code set it to be inheritable. Or else crazy errors show up because we end up fdopen()ing a random fd simply because the original fd isn't inherited.

I'd love to get this patch in. I rebased my patch on top of the current master.

Please let me know what else I can do to get this in. Thanks for all your help.

Contributor

unaizalakain commented May 17, 2014

six is a module wrapper that addresses the issue of having different package and module names in python 2 and python 3. In this particular case, socketserver is imported some lines beneath with from django.utils.six.moves import socketserver (https://github.com/dlamotte/django/blob/3328ff915d2524b8dab7a0668d116a6af82a2e6c/django/core/servers/basehttp.py#L34) so you could use that one ;-)

Ah, sure. I've updated the code to simply use the socketsever module in there. Thanks!

Contributor

unaizalakain commented May 18, 2014

Fine, looks perfect to me, thanks for your work! @timgraham, it works fine with python 2.7 and 3.4

@timgraham timgraham commented on an outdated diff May 28, 2014

docs/releases/1.7.txt
@@ -635,6 +635,10 @@ Management Commands
* All HTTP requests are logged to the console, including requests for static
files or ``favicon.ico`` that used to be filtered out.
+ * The development server will reuse the same socket across automatic reloads
+ which ensures no failed request due to reloading your browser too quickly.
@timgraham

timgraham May 28, 2014

Owner

"which ensures that requests will not fail if the browser is reloaded too quickly."
(also move to 1.8 release notes as 1.7 is feature frozen)

@timgraham timgraham commented on an outdated diff May 28, 2014

docs/ref/django-admin.txt
@@ -888,6 +888,16 @@ Example usage::
The development server is multithreaded by default. Use the ``--nothreading``
option to disable the use of threading in the development server.
+.. django-admin-option:: --nopersistsock
+
+.. versionadded:: 1.7
Owner

timgraham commented May 28, 2014

Please note our commit message guidelines (something like "Fixed #XXXXXX -- Made runserver persist socket across reloads"). Thanks.

Contributor

unaizalakain commented Jun 7, 2014

I could push the required updateds, what do you say @dlamotte?

Fixed #18855 -- Made runserver persist socket across reloads
Using livereload (livereload.com) with Django becomes painful when
updating a file immediately results in reloading the webpage AND
the Django dev server.  There is a short period of time when the dev
server is not listening as it is busy reloading (frequently hit
when using livereload).

This is exacerabated with larger projects as reload time is longer.

dlamotte commented Jun 7, 2014

Please feel free to push required updates in the future (I just updated it as well). I just want this in. I started trying to get it in 2 releases ago...

Owner

timgraham commented Jun 7, 2014

GitHub says the PR doesn't merge cleanly, did you rebase it?

Contributor

unaizalakain commented Jun 7, 2014

I understand you @dlamotte, sometimes it's a bit hard to get things in. Updated PR can be found here: #2773

@timgraham timgraham closed this Jun 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment