Code

Opened 6 years ago

Closed 9 months ago

#8413 closed Bug (duplicate)

Django runserver doesn't reload itself after updating files on disk that raised error on import.

Reported by: buriy Owned by: buriy
Component: Core (Management commands) Version: master
Severity: Normal Keywords: autoreload reload import error fix runserver
Cc: leosoto Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When autoreload is on,

  1. after any occasional error in any models.py file, reloading doesn't work for further updates of that file.
  2. modules that had import errors and updated on disk, are never reloaded.

Applied patch fixes the described problems and also allows to track various wrong usages of import API.

Attachments (2)

8413.patch (3.2 KB) - added by buriy 6 years ago.
Fix
8413_2.patch (2.1 KB) - added by buriy 6 years ago.
Better fix working through PEP 302

Download all attachments as: .zip

Change History (22)

Changed 6 years ago by buriy

Fix

comment:1 Changed 6 years ago by mtredinnick

  • milestone changed from 1.0 maybe to post-1.0
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

I'm -1 on this because changing the behaviour of __import__ for everything just for a couple of edge-cases with the auto-reloader doesn't seem worth it. But we can make an ultimate decision after 1.0.

comment:2 Changed 6 years ago by buriy

  • Owner changed from burchik@… to buriy

The patch will be rewritten to new hooks from PEP 302 today, it won't include import overriding.
Please don't judge without knowing all facts.

comment:3 Changed 6 years ago by buriy

Well, actually, you were absolutely correct in previous comment. Adding new hook that doesn't deal with import in any way, so it's safe to use.

Changed 6 years ago by buriy

Better fix working through PEP 302

comment:4 Changed 6 years ago by leosoto

  • Cc leosoto added

comment:5 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:7 Changed 3 years ago by gabrielhurley

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

comment:8 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:9 Changed 2 years ago by aaugustin

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

Accepting the patch because the submitter addressed Malcolm's comment.

Unfortunately the GitHub link is broken, and the previous patch needs updating — at least, we no longer need Python 2.3 compatibility.

comment:10 follow-up: Changed 2 years ago by aaugustin

This problem was also reported in #9589. This ticket has patches but their approach was rejected by a core dev and a BDFL.

comment:11 in reply to: ↑ 10 Changed 2 years ago by carljm

Replying to aaugustin:

This problem was also reported in #9589. This ticket has patches but their approach was rejected by a core dev and a BDFL.

I don't think that's accurate - the initial patch there was rejected (for good reason), but there are two subsequent patches that take quite a different (and, at least at first glance, much better) approach; there's no evidence those patches were ever given review. I think the second two patches on #9589 deserve another look if someone does look into fixing this.

Last edited 2 years ago by carljm (previous) (diff)

comment:12 Changed 2 years ago by aaugustin

Indeed, Carl is right. Whoever tackles this should review both tickets.

comment:14 Changed 19 months ago by tdhutt@…

This is really annoying. I briefly tested ebas's patch and it seems to work. Can somebody please apply it?

comment:15 Changed 19 months ago by tdhutt@…

PS: Here is the patch, since apparently github doesn't let you download them. Tested on django 1.4.1. The relavent file is in (usually) in /usr/local/lib/python2.7/dist-packages/django/utils/autoreload.py so you can apply it like this:

cd /usr/local/lib/python2.7/dist-packages/django/utils/autoreload.py
sudo patch autoreload.py ~/autoreload_patch.diff

where autoreload_patch.diff contains the following:

53a54

_error_files = []

57c58
< for filename in filter(lambda v: v, map(lambda m: getattr(m, "file", None), sys.modules.values())):
---

for filename in filter(lambda v: v, map(lambda m: getattr(m, "file", None), sys.modules.values())) + _error_files:

72a74,76

try:

del _error_files[_error_files.index(filename)]

except ValueError: pass

75a80,92

def check_errors(fn):

def wrapper(*args, kwargs):

try:

fn(*args, kwargs)

except (ImportError, IndentationError, NameError,

SyntaxError, TypeError), msg:

et, ev, tb = sys.exc_info()
if ev.filename not in _error_files:

_error_files.append(ev.filename)

raise

return wrapper

144,145c161,162
< reloader(main_func, args, kwargs)
<
---

wrapped_main_func = check_errors(main_func)
reloader(wrapped_main_func, args, kwargs)

comment:16 Changed 19 months ago by anonymous

Damnit. Try again:

53a54
> _error_files = []
57c58
<     for filename in filter(lambda v: v, map(lambda m: getattr(m, "__file__", None), sys.modules.values())):
---
>     for filename in filter(lambda v: v, map(lambda m: getattr(m, "__file__", None), sys.modules.values())) + _error_files:
72a74,76
>             try:
>                 del _error_files[_error_files.index(filename)]
>             except ValueError: pass
75a80,92
> def check_errors(fn):
>     def wrapper(*args, **kwargs):
>         try:
>             fn(*args, **kwargs)
>         except (ImportError, IndentationError, NameError, 
>                 SyntaxError, TypeError), msg:
>             et, ev, tb = sys.exc_info()
>             if ev.filename not in _error_files:
>                 _error_files.append(ev.filename)
> 
>             raise
>     return wrapper
> 
144,145c161,162
<     reloader(main_func, args, kwargs)
< 
---
>     wrapped_main_func = check_errors(main_func)
>     reloader(wrapped_main_func, args, kwargs)

comment:17 Changed 19 months ago by tdhutt@…

After more extensive testing, it's working quite well but I did get this one error:

Unhandled exception in thread started by <function wrapper at 0x1f6e8c0>
Error in sys.excepthook:
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/apport_python_hook.py", line 50, in apport_excepthook
    if not enabled():
TypeError: 'NoneType' object is not callable

Original exception was:
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/utils/autoreload.py", line 83, in wrapper
    fn(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/django/core/management/commands/runserver.py", line 111, in inner_run
    ipv6=self.use_ipv6, threading=threading)
  File "/usr/local/lib/python2.7/dist-packages/django/core/servers/basehttp.py", line 253, in run
    httpd.serve_forever()
  File "/usr/lib/python2.7/SocketServer.py", line 225, in serve_forever
    r, w, e = select.select([self], [], [], poll_interval)
AttributeError: 'NoneType' object has no attribute 'select'

comment:18 Changed 17 months ago by ramiro

  • Keywords runserver added
  • Summary changed from Django doesn't reload itself after updating files on disk that raised error on import. to Django runserver doesn't reload itself after updating files on disk that raised error on import.

comment:19 Changed 17 months ago by ramiro

  • Patch needs improvement unset

I've updated the patch contributed by berto to #9589: https://github.com/ramiro/django/compare/8413_9589_reload_runserver

Testing of the patch on Unixy platforms (including Mac OS X) is welcome. I will try to test it on Windows.

I plan to commit this to master in a few days.

comment:20 Changed 9 months ago by timo

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

Closing as duplicate of #9589 - it looks like the fix was committed with a reference to that ticket but not this one so it wasn't closed automatically.

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.