Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the imported app module from sys.modules on SyntaxError #2588

Conversation

segevfiner
Copy link

@segevfiner segevfiner commented Jan 8, 2018

When a SyntaxError occurrs in a package's __init__.py file, Python seems to leave an empty module in sys.modules. This will result in any request after the first to give you a NoAppException instead of the SyntaxError again.

I'm not 100% sure this doesn't have any side effects... It's an hack... So please review and tell me what you think about this. (Maybe more conditions for triggering this?)

Fixes #2423

Sample

In hello/__init__.py:

from flask import Flask
app = Flask(__name__)

@app.route('/')
def hello_world():
    return 'Hello, World!'
set FLASK_APP=hello
set FLASK_DEBUG=1
flask run

Than edit __init__.py and introduce a syntax error.

When a SyntaxError occurrs in a package's `__init__.py` file, Python
seems to leave an empty module in `sys.modules`. This will result in any
request after the first to give you a `NoAppException` instead of the
`SyntaxError` again.

Fixes pallets#2423
@davidism
Copy link
Member

davidism commented Jan 8, 2018

Is there an easy way to write a test for this? Otherwise, seems fine to me.

@mitsuhiko
Copy link
Contributor

This does not just apply for packages, it affects a boatload of situations and this only catches on case. I'm not sure what the best approach here is.

@mitsuhiko
Copy link
Contributor

One way would be to patch __import__ when the reloader is active and catch down ImportErrors and clean up recursively.

@segevfiner
Copy link
Author

Hmm. I see... This won't catch sub-packages that fail to import on SyntaxError, only the main app package. As such, they will be left empty, and on the next request the app will get them as an empty module. Flask won't fail to find the app, but the app will fail to find stuff in that package...

Seriously that Python behavior is so off...

@davidism
Copy link
Member

davidism commented Jan 8, 2018

Maybe modify the Werkzeug reloader to record the loaded modules before starting the reloader, then reset them before triggering a reload.

@davidism
Copy link
Member

davidism commented Jan 9, 2018

I can't reproduce this on Python 2.7 or 3.6, Flask 0.12 and master. I tried starting with valid syntax then editing in an error, and vice versa.

@davidism
Copy link
Member

davidism commented Jan 9, 2018

The test you added passes without your patch.

@segevfiner
Copy link
Author

segevfiner commented Jan 9, 2018 via email

@davidism
Copy link
Member

davidism commented Jan 9, 2018

I've had developers try to reproduce this on Windows, Mac, and Linux with and without the Watchdog reloader. It works for all of them.

@davidism
Copy link
Member

davidism commented Jan 9, 2018

The normal Python importer is careful about cleaning up after failed imports. It's likely that something else in your stack is affecting this. I'm going to close this for now, but if you can figure out what's going on I can reopen it later.

@mitsuhiko
Copy link
Contributor

@davidism there are long running bugs in the interpreter where cleanup does not correctly happen. It's a limitation of the python importer.

@segevfiner
Copy link
Author

I went with addressing the underlying issue in CPython python/cpython#5142.

@davidism
Copy link
Member

davidism commented Jan 9, 2018

I'm not doubting that the error happens for you, but I'm curious why no one I asked was able to reproduce this in a variety of environments. I think getting cpython to fix it is the right course of action.

@segevfiner
Copy link
Author

@davidism Yeah I got curious about that too. Which is why I went ahead and debugged it. The bug is obviously in 2.7 but not in 3.6. I literally went in with a debugger and saw the exact flow of code that causes this. It's hard to tell why they weren't able to reproduce it though.

@kinow kinow mentioned this pull request Sep 6, 2018
6 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reloader fails to reload after SyntaxError when the app is in an __init__.py file
3 participants