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.

Sign up for GitHub

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

Register flask.current_app.teardown_appcontext only once #120

Closed
wants to merge 2 commits into from
+13 −6
Closed

Register flask.current_app.teardown_appcontext only once #120

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ddbd22d
SQL update: only register the shutdown_session function once to the F…
jsarchibald Jun 1, 2020
cfc41df
Register flask.current_app.teardown_appcontext only once per connecti…
jsarchibald Jun 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
SQL update: only register the shutdown_session function once to the F… 
…lask teardown_appcontext_funcs. Checks existing teardown functions before registering it.
@jsarchibald
jsarchibald committed Jun 1, 2020
commit ddbd22ded1a681d586aed2c947fc90d8bb3f6e8b
16 changes: 11 additions & 5 deletions src/cs50/sql.py
View file
Original file line number Diff line number Diff line change
@@ -286,11 +286,17 @@ def execute(self, sql, *args, **kwargs):
# Connect now
flask.g._connection = self._engine.connect()

# Disconnect later
@flask.current_app.teardown_appcontext
def shutdown_session(exception=None):
if hasattr(flask.g, "_connection"):
flask.g._connection.close()
# Disconnect later - but only once
teardown = True
for func in flask.current_app.teardown_appcontext_funcs:
if func.__name__ == "shutdown_session":
teardown = False

if teardown:
@flask.current_app.teardown_appcontext
def shutdown_session(exception=None):
if hasattr(flask.g, "_connection"):
flask.g._connection.close()
Copy link
Member

@kzidane kzidane Jun 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would make more sense for the flag to be set on the execute function itself such that it could do something like:

def execute(...):
  ...
 
  # Ensure shutdown_session is just added once
  if not execute.teardown_appcontext_added:
    execute.teardown_appcontext_added = True

    # Register shutdown_session on app context teardown    
    @flask_current_app.teardown_appcontext
    def shutdown(...):
      ...

# teardown_appcontext handler is not registered initially
execute.teardown_appcontext_added = False

instead of having to search through the list of teardown_appcontext_funcs repeatedly? Also, fwiw, I don't think function names uniquely identifies a function in this case (i.e., more than one teardown_appcontext handler could have the same name).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created an instance variable, teardown_appcontext_added, to the SQL class to do this.


# Use this connection
connection = flask.g._connection
2 changes: 1 addition & 1 deletion tests/flask/application.py
View file
Original file line number Diff line number Diff line change
@@ -9,7 +9,7 @@

app = Flask(__name__)

db = cs50.SQL("sqlite:///../sqlite.db")
db = cs50.SQL("sqlite:///../test.db")

@app.route("/")
def index():