Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#29044 closed New feature (wontfix)

Changes to autoreload to make atexit calls work in the child process.

Reported by: Alex Orange Owned by: nobody
Component: Core (Management commands) Version: 2.0
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently the child process is killed if the parent is interrupted (i.e. CTRL+C). This means in turn that atexit handlers are not called. This traces to the use of subprocess.call which kills the child process if it catches an exception (KeyboardInterrupt for instance). This is fixed if we use Popen instead (which is what call does) and instead of killing the child process, simply wait for it to complete. My fix can been seen here: https://github.com/CrazyCasta/django/commit/cfbb6e2f5e95733b8a2448cef18227e56d6e37b5 but I need some help.

Two things:

  1. Feedback on how best to handle this. It is my opinion that the best way to handle this is to the child process finish itself up however long that might take and then eventually exit. If there is some sort of bug in the code (users' or Django's) that prevents this from the child process from completing then the bug should be fixed. Another way to handle this would be to timeout the p.wait in the exception with some configurable delay and then kill and wait.
  1. How should this be tested. I'm guessing that the best way to do this would be to create a very simple command, create a subprocess that calls that command, wait for the grand-child process to startup, send the child process an interrupt and wait for the grand-child process to complete. It appears that the current code avoids creating any child processes. Is there a wish not to create child processes in the tests, or would it be ok to do so as long as it completes quickly (i.e. don't use runserver, create a dummy command that will run much faster).

Change History (2)

comment:1 by Alex Orange, 6 years ago

Needs tests: set

Setting needs tests flag. As I mentioned above, I need some feedback on what the proper way of testing this would be.

comment:2 by Ramiro Morales, 6 years ago

Resolution: wontfix
Status: newclosed

We are trying to change completely the development webserver autoreloader. See #27685 and the related PR, so I suspect no further features will be added to the current code.

We are finding the same problem there as you: How to test this code in presence of multiple processes, timing constraints, etc.? If you can contribute somehow to that effort e.g. by testing your use case and with ideas/code about how to implement the tests it will be greatly appreciated.

Last edited 6 years ago by Ramiro Morales (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top