[Django] #30416: Runserver's reloading mechanism should restore terminal state completely
Django
-------------------------------------+-------------------------------------
Reporter: Daniel | Owner: nobody
Hahler |
Type: Bug | Status: new
Component: Core | Version: 2.2
(Management commands) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Currently it only ensures that echo is on, but not that e.g. Ctrl-C works
etc.
This can be triggered when using pdb++, which uses pyrepl itself: it puts
the terminal into raw state.
If the reloader then kicks in while at the prompt this will not be
restored (except for echo).
Related issue, which mentions that a better mechanism would be good:
https://code.djangoproject.com/ticket/15880
--
Ticket URL: <https://code.djangoproject.com/ticket/30416>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Management | Version: 2.2
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> Currently it only ensures that echo is on, but not that e.g. Ctrl-C works
> etc.
>
> This can be triggered when using pdb++, which uses pyrepl itself: it puts
> the terminal into raw state.
>
> If the reloader then kicks in while at the prompt this will not be
> restored (except for echo).
>
> Related issue, which mentions that a better mechanism would be good:
> https://code.djangoproject.com/ticket/15880
New description:
Currently it only ensures that echo is on, but not that e.g. Ctrl-C works
etc.
This can be triggered when using pdb++, which uses pyrepl itself: it puts
the terminal into raw state.
If the reloader then kicks in while at the prompt this will not be
restored (except for echo).
Related issue, which mentions that a better mechanism would be good:
https://code.djangoproject.com/ticket/15880
btw: pyrepl has no chance to restore itself: listening to SIGTERM is not
possible in threads apparently, and e.g. `atexit` is also not triggered
there.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/30416#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Management | Version: 2.2
commands) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Hi Daniel. I'm going to Accept this, because I see where you're coming
from, but I would be grateful if you could add a detailed set of reproduce
steps and/or an example project so that someone picking it up can get
going on it easily.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/30416#comment:2>
Django
-------------------------------------+-------------------------------------
Component: Core (Management | Version: 2.2
commands) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => needsinfo
Comment:
Without more details (after 7 months) this isn't really actionable. Happy
to see it taken on, with a concrete action plan in place.
--
Ticket URL: <https://code.djangoproject.com/ticket/30416#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Core (Management | Version: 2.2
commands) |
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Daniel Hahler):
@Carlton
> I would be grateful if you could add a detailed set of reproduce steps
and/or an example project so that someone picking it up can get going on
it easily.
I think everybody being able to fix this has enough info in the original
issue already.
I've mentioned one way to trigger this: use pdb++ (with pyrepl), then have
the reloader kick in while in the debugger.
A simpler method would be using ``termios.tcsetattr`` directly to put it
into e.g. raw state, then have the reloader kick in.
Therefore I suggest re-opening it already - it's an existing issue after
all, and was mentioned in another issue (linked above) already.
--
Ticket URL: <https://code.djangoproject.com/ticket/30416#comment:4>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Core (Management | Version: 2.2
commands) |
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Daniel Hahler):
Also note that the reloader should not really kick in in the first place
when the debugger is used / only when the request finished.
(The use case is having a ``pdb.set_trace()``, the debugger being used,
and then saving some file - it should not "kill" the debugger then
(resulting in this issue))
Therefore waiting for the request / debugger to be finished before
reloading would work around this issues (allowing the debugger (pyrepl in
this case) to restore the terminal state itself).
Related: https://github.com/pallets/werkzeug/pull/1525 (for Werkzeug's
reloader, which is similar IIRC).
--
Ticket URL: <https://code.djangoproject.com/ticket/30416#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Management | Version: 2.2
commands) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: needsinfo =>
Comment:
I've rebased and added a test to
https://github.com/django/django/pull/11291.
To reproduce this:
1. install pdbpp (with pyrepl)
2. put `pdb.set_trace()` in a view
3. `manage.py runserver`
4. trigger the reloader or use ctrl-c directly
5. see that ctrl-c does not work on your shell anymore until you use
`reset`
Alternatively:
1. add the following code to a view or similar:
{{{
import sys
import termios
fd = sys.stdin.fileno()
attr = termios.tcgetattr(fd).copy()
attr[3] &= ~(
termios.ICANON | termios.ECHO | termios.IEXTEN | (termios.ISIG
* 1)
)
termios.tcsetattr(fd, termios.TCSANOW, attr)
}}}
2. trigger the code
3. observer that ctrl-c does not work anymore
With the patch/fix it will work again after the reloader gets triggered,
and/or when killing the runserver manually.
--
Ticket URL: <https://code.djangoproject.com/ticket/30416#comment:6>
Django
-------------------------------------+-------------------------------------
| Hahler
Type: Bug | Status: assigned
Component: Core (Management | Version: 2.2
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Daniel Hahler
* status: new => assigned
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30416#comment:7>
Django
-------------------------------------+-------------------------------------
| Hahler
Type: Bug | Status: assigned
Component: Core (Management | Version: 2.2
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Tom Forbes (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/30416#comment:8>
Django
-------------------------------------+-------------------------------------
| Hahler
Type: Bug | Status: assigned
Component: Core (Management | Version: 2.2
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 0 => 1
Comment:
I've left
[https://github.com/django/django/pull/11291#pullrequestreview-625215987
comments on PR] — ultimately I'm not sure we need to resolve this
ourselves…
Maybe offering a way to pause the auto-reloader if you're in a debugger
(as per comment:5 — but not necessarily with the lock approach in linked
PR), but even then, I'm not sure we're committed to the auto-reloader
working flawlessly if you save file changes whilst working in the
debugger.
Possibly the suggested patch is a marginal improvement, but there are no
tests at all. (Indeed some coverage is removed.) I'm not sure if we can
come up with decent tests to make sure we don't introduce regressions.
(It's on that untestable edge.) If we do merge this I think we need to ask
regular contributors to try and break it with their debugging setup just
to be sure we didn't break something else unexpectedly.
Current status ≈-0
--
Ticket URL: <https://code.djangoproject.com/ticket/30416#comment:9>