Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#24704 closed Bug (fixed)

Development server does not restart on SynaxError in models.py

Reported by: Artem Rizhov Owned by: Aymeric Augustin
Component: Core (Management commands) Version: 1.7
Severity: Normal Keywords:
Cc: artem.rizhov@… 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

Django documentation states:

each time you change Python code while the server is running, the server will check your entire Django project for errors (see the check command). If any errors are found, they will be printed to standard output, but it won’t stop the server.

https://docs.djangoproject.com/en/1.7/ref/django-admin/#runserver-port-or-address-port

However in my case the server process finishes with exit code 1 when syntax error appears in admin.py or models.py. Below is the error message that I see in my console.

  File "/home/.../src/core/admin.py", line 16
    foo =
        ^
SyntaxError: invalid syntax

So it seems there is either a bug or a mistake in the documentation.

Django 1.7.5
Python 2.7.6
virtualenv 1.11.4
Ubuntu 14.04.2 LTS

Attachments (1)

24704.diff (890 bytes) - added by Tim Graham 4 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 4 years ago by Tim Graham

Resolution: invalid
Status: newclosed

A SyntaxError is a Python concept, not something relevant to the check command. When writing the docs, I'd like to assume enough familiarity with Python that we don't need to explain that a program can't run if it contains syntax errors. Hope that explanation helps.

comment:2 in reply to:  1 Changed 4 years ago by Artem Rizhov

Resolution: invalid
Status: closednew

Thank you for your explanation. To be honest, it does not help at all. The question is not about the program, but about the dev server which hosts it.

Please note that the server does not exit when syntax error happens in views.py. So, it really makes sense to explain how you did that impossible thing! ;) I'm joking, please don't waste your time for explanation of this. But please believe many perfectionists have a lot of another work and do not have time to dive into the Django core.

The server and the application are two different entities. I don't think this should be clear that the server can not handle syntax errors in the app. Furthermore, it does in some cases. And even furthermore, I'm sure it is possible to implement such feature for any cases, even with Python, for example, with a master process which is watching and rerunning the app when it exits with error.

When I read documentation I'd want to believe that the written text states true. If not, why not to add a note about this? I found few tickets about this problem with both fixed and rejected statuses. So I came to the documentation while searching for the truth. But it lies :) Just replace "any errors" with something more honest. I'd not spend time to write this ticket if the doc contains notes about this, and you'd not spend time to answer to my question. And I'm sure many people will thank you.

But it would be really better if Django dev server can handle any errors. I've started learning this issue because I've made too many manual restarts of the dev server while working on site admin area.

The simplest workaround is while true; do ./manage.py runserver ; sleep 1 ; done. But it does not work well with PyCharm which I really love.

Last edited 4 years ago by Artem Rizhov (previous) (diff)

comment:3 Changed 4 years ago by Artem Rizhov

Maybe I've selected wrong component for this issue? Does it make sense to change it from "Core (System checks)" to "Management commands" or "Other"?

Changed 4 years ago by Tim Graham

Attachment: 24704.diff added

comment:4 Changed 4 years ago by Tim Graham

Has patch: set

Does the attached patch help?

comment:5 Changed 4 years ago by Artem Rizhov

Yes, this version seems much more clear! Thank you!

comment:6 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 0f2e82b9:

Fixed #24704 -- Clarified system check interaction with runserver.

comment:7 Changed 4 years ago by Tim Graham <timograham@…>

In 8fa763a:

[1.8.x] Fixed #24704 -- Clarified system check interaction with runserver.

Backport of 0f2e82b9ec9797147945a6f9a402b5ae0fb1d9f4 from master

comment:8 Changed 4 years ago by Tim Graham <timograham@…>

In f6ecb540:

[1.7.x] Fixed #24704 -- Clarified system check interaction with runserver.

Backport of 0f2e82b9ec9797147945a6f9a402b5ae0fb1d9f4 from master

comment:9 Changed 4 years ago by Artem Rizhov

Cc: artem.rizhov@… added

comment:10 Changed 4 years ago by Tim Graham

Summary: Development server do not restart on SynaxErrorDevelopment server does not restart on SynaxError

comment:11 Changed 4 years ago by Tim Graham

Component: Core (System checks)Core (Management commands)
Has patch: unset
Resolution: fixed
Status: closednew
Summary: Development server does not restart on SynaxErrorDevelopment server does not restart on SynaxError in models.py
Triage Stage: UnreviewedAccepted

It seems that before 0d2c8ff2be733c7cc83a023bbafe0258faa5603c syntax errors in models.py didn't stop the server. Now it does:

Traceback (most recent call last):
  File "manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/home/tim/code/django/django/core/management/__init__.py", line 428, in execute_from_command_line
    utility.execute()
  File "/home/tim/code/django/django/core/management/__init__.py", line 420, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/tim/code/django/django/core/management/__init__.py", line 288, in fetch_command
    commands = get_commands()
  File "/home/tim/code/django/django/core/management/__init__.py", line 125, in get_commands
    apps.populate_models()
  File "/home/tim/code/django/django/apps/registry.py", line 123, in populate_models
    app_config.import_models(all_models)
  File "/home/tim/code/django/django/apps/base.py", line 164, in import_models
    self.models_module = import_module(models_module_name)
  File "/home/tim/.virtualenvs/django34/lib/python3.4/importlib/__init__.py", line 109, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 2231, in _gcd_import
  File "<frozen importlib._bootstrap>", line 2214, in _find_and_load
  File "<frozen importlib._bootstrap>", line 2203, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 1200, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 1129, in _exec
  File "<frozen importlib._bootstrap>", line 1444, in exec_module
  File "<frozen importlib._bootstrap>", line 1549, in get_code
  File "<frozen importlib._bootstrap>", line 1509, in source_to_code
  File "<frozen importlib._bootstrap>", line 321, in _call_with_frames_removed
  File "/home/tim/code/mysite/polls/models.py", line 15
    class MyModelmodels.Model):

Not sure if the old behavior can be restored, but reopening in case someone would like to investigate.

comment:12 Changed 4 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

Aaargh.

comment:13 Changed 4 years ago by Aymeric Augustin

Has patch: set

I had a look at this. Unfortunately our reloading technique is to watch imported Python files which makes things complicated (in addition to requiring hacks for everything that isn't a Python file).

See this PR for a possible "solution".

comment:14 Changed 4 years ago by Aymeric Augustin

I'm not proud of that PR but I think it's an improvement and it should go in until we reconsider the autoreloader's design.

comment:15 Changed 4 years ago by Tim Graham

Patch needs improvement: set

comment:16 Changed 4 years ago by Aymeric Augustin

Patch needs improvement: unset

Patch was improved.

Since it can be a real inconvenience in day-to-day development and it can be considered a regression in 1.7, it would make sense to backport at least to 1.8 (LTS). The patch doesn't apply cleanly but shouldn't be very hard to backport.

If I want to backport, I have to add an entry in the release notes for 1.8.5, anything else?

comment:17 Changed 4 years ago by Tim Graham

Patch needs improvement: set

Just release notes are required for the backport.

Marking as "Patch needs improvement" as it seems the patch still doesn't work if pyinotify is installed. It does work on Windows.

comment:18 Changed 4 years ago by Aymeric Augustin

Patch needs improvement: unset

comment:19 Changed 4 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:20 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

Resolution: fixed
Status: assignedclosed

In fe6ddb8:

Fixed #24704 -- Made the autoreloader survive SyntaxErrors.

With this change, it's expected to survive anything except errors
that make it impossible to import the settings. It's too complex
to fallback to a sensible behavior with a broken settings module.

Harcoding things about runserver in ManagementUtility.execute is
atrocious but it's the only way out of the chicken'n'egg problem:
the current implementation of the autoreloader primarily watches
imported Python modules -- and then a few other things that were
bolted on top of this design -- but we want it to kick in even if
the project contains import-time errors and django.setup() fails.

At some point we should throw away this code and replace it by an
off-the-shelf autoreloader that watches the working directory and
re-runs django-admin runserver whenever something changes.

comment:21 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

In b79fc11:

Made the autoreloader survive all exceptions.

Refs #24704.

comment:22 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

In cc14d51:

Fixed #24704 -- Made the autoreloader survive SyntaxErrors.

With this change, it's expected to survive anything except errors
that make it impossible to import the settings. It's too complex
to fallback to a sensible behavior with a broken settings module.

Harcoding things about runserver in ManagementUtility.execute is
atrocious but it's the only way out of the chicken'n'egg problem:
the current implementation of the autoreloader primarily watches
imported Python modules -- and then a few other things that were
bolted on top of this design -- but we want it to kick in even if
the project contains import-time errors and django.setup() fails.

At some point we should throw away this code and replace it by an
off-the-shelf autoreloader that watches the working directory and
re-runs django-admin runserver whenever something changes.

Backport of fe6ddb837d from master

comment:23 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

In 2b08b364:

[1.8.x] Made the autoreloader survive all exceptions.

Refs #24704.

Backport of b79fc11d73 from master

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