Code

Opened 5 years ago

Closed 6 months ago

#9722 closed New feature (fixed)

Use pyinotify (where available) instead of polling filesystem every second to detect changes

Reported by: lamby Owned by: unaizalakain
Component: Core (Management commands) Version: master
Severity: Normal Keywords:
Cc: unai@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This patch series adds support for pyinotify to replace the "poll-every-one-second" mechanism in django.utils.autoreload on systems where that library is available.

Inotify is an event-driven notifier for monitoring filesytems changes. Instead of dancing around the filesystem comparing modification times every second, we can simply register all the files we are interested in and let the thread block until one of them changes. When this happens, we reload the server as before.

This has the following advantages over the current polling system:

  • Scales far better to larger projects.
  • Response time to code changes is drastically reduced - the server restarts pretty much instantly instead of having to wait ~0.5 seconds for the reload.
  • More robust change detection - pretty much all forms of modification of the interested file are noticed (in particular, moving the file and editing the file without changing the mtime)
  • Saves battery usage - I have to use --noreload on my laptop (with encrypted filesystems) as it noticably reduces my battery life. Django is killing kittens.
  • Cleaner implementation - Polling is just dirty, and requires us to maintain a huge dict full of modification times.

Whilst inotify is Linux-specific, the patch gracefully degrades if a suitable version of pyinotify is not available. It does not add pyinotify as a dependency to Django.

Attachments (3)

0001-Refactor-functionality-to-get-interesting-files-into.patch (1.2 KB) - added by lamby 5 years ago.
0002-Add-support-for-pyinotify.patch (3.2 KB) - added by lamby 5 years ago.
Update 0002-Add-support-for-pyinotify.patch
0001-add-support-for-pyinotify.patch (3.2 KB) - added by ch0wn 4 years ago.
Update for django 1.2.1 and pyinotify 0.7

Download all attachments as: .zip

Change History (27)

comment:1 Changed 5 years ago by lamby

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

This doesn't yet for modules that are lazily loaded - need to hook into the request_finished signal to update the files inotify is waiting on, otherwise we will never update the watch list (as we are blocking on it forever). The inotify interface will get in the way a bit here - will probably require calling oll ourselves with an additional anonymous file descriptor which (if written to) will tell us to update our watch list.

comment:2 Changed 5 years ago by lamby

  • Has patch unset

Changed 5 years ago by lamby

Update 0002-Add-support-for-pyinotify.patch

comment:3 Changed 5 years ago by lamby

  • Has patch set

No need for file descriptor nonsense; we can simply update the inotify watch list from the thread the request_finished callback is called from. Anyway, current patch fixes the lazily loaded modules issue.

comment:4 Changed 5 years ago by jacob

  • Triage Stage changed from Unreviewed to Design decision needed

Changed 4 years ago by ch0wn

Update for django 1.2.1 and pyinotify 0.7

comment:5 Changed 4 years ago by ch0wn

I combined and updated the previous patches and attached one that works with django 1.2.1 and latest pyinotify.

comment:6 Changed 3 years ago by gabrielhurley

  • Component changed from django-admin.py runserver to Core (Management commands)

comment:7 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

comment:8 Changed 2 years ago by aaugustin

  • Easy pickings unset
  • Triage Stage changed from Design decision needed to Accepted
  • UI/UX unset

comment:9 Changed 6 months ago by unaizalakain

  • Cc unai@… added
  • Needs tests set

comment:10 Changed 6 months ago by aaugustin

  • Needs documentation set
  • Patch needs improvement set

I left a few comments on the pull request. Overall this looks pretty good.

It still needs documentation. I'm not sure how we can write tests.

comment:11 Changed 6 months ago by unaizalakain

I squashed the suggested changes into the old commit, please, let me know your opinion.

I'm sure how to test this either. Were do you suggest to add documentation?

comment:13 Changed 6 months ago by unaizalakain

OK, added a short sentence stating that pyinotify is used for autoreloading when available.

comment:14 Changed 6 months ago by unaizalakain

  • Owner changed from nobody to unaizalakain
  • Status changed from new to assigned

comment:15 Changed 6 months ago by unaizalakain

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:16 Changed 6 months ago by timo

  • Needs documentation set

Please don't mark your own patch Ready for Checkin -- another person who reviews the patch should do that. I think this could use a bit more documentation as described in the PR.

comment:17 Changed 6 months ago by timo

  • Triage Stage changed from Ready for checkin to Accepted

comment:18 Changed 6 months ago by unaizalakain

Sorry for the bad ticket handling, won't happen again. PR updated with suggested changes.

comment:19 Changed 6 months ago by aaugustin

  • Needs documentation unset
  • Triage Stage changed from Accepted to Ready for checkin

The pull request looks good to me.

However I can't really try it because I'm not using Linux for my development environment.

I would also suggest that the committer summarize and tone down the justification for the feature; it's a bit over the top right now eg. "Django is killing kittens". I get the humor, but not everyone will!

comment:20 Changed 6 months ago by aaugustin

(I'm interested because I'd like to implement the equivalent behavior with kqueue on OS X, and this patch lays the groundwork.)

comment:21 Changed 6 months ago by unaizalakain

When I copied the original justification, I wasn't sure what to do with that sentence :p It's now deleted ;)

comment:22 Changed 6 months ago by claudep

Apart from minor cleanup with trailing spaces, I can confirm the patch is working well on my Linux machine.

comment:23 Changed 6 months ago by unaizalakain

  • Version changed from 1.0 to master

comment:24 Changed 6 months ago by Tim Graham <timograham@…>

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

In 15f82c701161b327cef54b469c00b6cbe01534db:

Fixed #9722 - used pyinotify as change detection system when available

Used pyinotify (when available) to replace the "pool-every-one-second"
mechanism in django.utils.autoreload.

Thanks Chris Lamb and Pascal Hartig for work on the patch.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.