Tornado autoreload feature will auto-reload into a syntax error and crash

[timabbott]

Zulip uses both Tornado and Django as part of our project, using the built-in autoreload tooling for both projects. We've for a long time been plagued by an annoying issue where whenever one does a rebase with merge conflicts, the Django process happily avoids reloading itself until those are fixed, but the Tornado process will reload into a state with syntax errors, crashing itself, and requiring a manual restart by the developer.

After doing some code reading, the problem here is that using e.g. the autoreload=True option in tornado.web.Application just doesn't have any code to do a syntax check of the modified modules before attempting to reload (the problem here is primarily not the _reload_attempted logic for trying just once; the problem is that the process dies when it tries to reload). It seems like fixing that would completely solve this problem.

I wrote a basically working patch to Tornado's autoreload module (which we've temporarily copied into Zulip) that we're using to solve this problem for us. The code is here: https://github.com/zulip/zulip/commit/ae0a9299880c79a6614f0a6c8eb8f3b39cbe628b; it'd need some cleanup before it could be integrated into mainline Tornado (mostly noted in the commit message, but I'll repeat it here):

• I dropped _watched_files support to make it a bit easier to implement which is OK for my project but not for others; since we don't have a syntax checker for those, probably we just need those to keep the old behavior.
• I don't think importlib.reload is the right function to call for doing the "check import" inside Python. I think it won't actually break anything important in most cases, but ideally we'd replace that with something that's always correct (possibly just using importlib.import_module after clearing the importlib cache for the target file?).
• It generally needs more testing.

Much as I'd like to, I don't have time to do that cleanup and contribute this to Tornado upstream in the near future, but I hope these notes are helpful for someone who does have time. I'm happy to help with testing a PR for this feature, though.

[bdarnell]

This is why the command-line wrapper mode of autoreload exists. The idea is that you can prefix your command line with python -m tornado.autoreload and then when you have a syntax error (or other startup failure - a typo in a main() function definition might be syntactically valid but fail at startup) there is "external" autoreload logic to catch it and try again. This is not documented or promoted as much as it should be, and it's had some bugs (some of which i've just fixed in the last few days), but it's my intended solution to this problem.

A pre-reload sanity check is not a bad idea either.

[andersk]

The python -m tornado.autoreload wrapper has a feature where it continues watching after the inner program exits (either by reaching the end of the code or by raising SystemExit). That’s convenient if you want to automatically rerun a test script, but seems quite inconvenient for a server program that listens until the user presses Ctrl+C.

• If the program does nothing special and allows the KeyboardInterrupt to propagate, an ugly traceback gets spewed to the terminal. Example: python3 -m tornado.autoreload -m wsgiref.simple_server.
• If the program catches the KeyboardInterrupt and attempts to exit normally, the autoreload wrapper won’t exit until a second Ctrl+C press. Example: python3 -m tornado.autoreload -m http.server.

Can that feature be made optional?

[bdarnell]

If the program does nothing special and allows the KeyboardInterrupt to propagate, an ugly traceback gets spewed to the terminal. Example: python3 -m tornado.autoreload -m wsgiref.simple_server.

Isn't that what happens without tornado.autoreload? Or is there something that catches KeyboardInterrupt by default these days? I personally find the stack trace on Ctrl-C to be pretty useful on occasion so I'd rather not see it suppressed.

Can that feature be made optional?

An option to break out of the reload loop on exit status 0 seems reasonable.

[andersk]

I’m not asking you to suppress the traceback on Ctrl-C exit. I’m just asking that if I write a program that suppresses it, I should be able to run it with tornado.autoreload in a way that still exits.

[bdarnell]

I've found an older issue #1239 which is essentially the same as this one. I'm going to close this one as a duplicate of the older issue (although the conversation here is more recent and may be useful).

[bdarnell]

Actually I'm going to reopen this but retitle it to be about the behavior raised in https://github.com/tornadoweb/tornado/issues/2398#issuecomment-1071789709: This issue is now about how python -m tornado.autoreload handles programs that exit cleanly with exit status 0. tornado.autoreload currently catches these exits and sits there waiting for a file to change (because I implemented this mode with unit test runners in mind), while you'd like it to just exit immediately. Both behaviors are reasonable, so we should think about some way to choose between them.