Remove SQLALCHEMY_COMMIT_ON_TEARDOWN

[immunda]

No description provided.

[gregeinfrank]

@immunda Why is SQLALCHEMY_COMMIT_ON_TEARDOWN considered dangerous? Can someone explain why this is being removed? Thanks!

[fuhrysteve]

@immunda Can you provide some insight on why this is being removed, and what the recommended approach is here?

[arbinish]

Guess this is removed just from the documentation, can still see the param in source (lines 728, 749)
https://github.com/mitsuhiko/flask-sqlalchemy/blob/master/flask_sqlalchemy/__init__.py

[mattupstate]

I'd guess that the reason is due to the teardown_appcontext callback carrying a bug that, even if you catch an exception during the app context, the response_or_exc will never be None. In other words, teardown_appcontext suffers from a general Python exception handling bug.

[fuhrysteve]

@mattupstate Makes sense.. it feels weird to have to commit explicitly at the end of every request.. although after doing it manually everywhere, I admit that I tend to like the fine grained control over when things get committed. To be fair, I never liked the django autocommit philosophy either.. feels like you're just trading inconvenience for sloppiness.

[immunda]

Sorry for the silence on this. Yep, that's the motivation, moving away from the (flawed) magic. I'm waiting to deprecate it entirely (3.0), because there's plans to introduce a more explicit transaction decorator first.

[jeffwidman]

For those who were still confused after reading the comments on this issue (like me), here's a more extensive explanation:
http://stackoverflow.com/questions/23301968/invalid-transaction-persisting-across-requests

As well as this PR: pallets/flask#1451

[roganov]

Reading @jeffwidman comment helped to understand the issue, but I'm still confused what this means for flask-sqlalchemy. Is there going to be another (similar) solution or do users have to commit manually now (which would be quite unfortunate, don't want to mix persistence and application logic even more)?

[justanr]

@roganov A handler with a try-finally would solve the problem. I'd suggest registering it as a request teardown handler unless you absolutely need the connection open afterwards. That said, I'm not sure how that'd interact with tools like Flask-DebugToolbar. And disable COMMIT_ON_TEARDOWN.

However, I'm also of the opinion that transaction management is part of persistence handling. And that persistence and application logic are two different concerns, but this isn't the forum for me going on about that.

[benasocj]

Recently PR pallets/flask#1822 got merged into Flask.
Will this maybe change the fact whether SQLALCHEMY_COMMIT_ON_TEARDOWN will still be removed in future?

[davidism]

Yeah, it's safer now, but I really just don't like the pattern. Explicit is better than implicit, etc.

Commit on teardown only moves the last call to db.commit() out of the view function, which doesn't seem like much of a burden to begin with. It also means that the developer can no longer handle commit errors, they would have to do the commit themselves in order to wrap it, at which point commit on teardown is useless.

I'm still for removing this, but open to discussion.

[asteinlein]

I disagree with removing it. Having db.commit() at the end of basically every view is very anti-DRY. Yes, I could add my own teardown function or some other hook, but I agree with i.e. @justanr that this belongs to persistence handling that this library is supposed to help us with.

[davidism]

That's not what DRY means.

[asteinlein]

Don't Repeat Yourself. db.commit() in every view is repeating yourself, when it might as well be abstracted away.

[davidism]

Committing during teardown moves an important part of the view to after the response is generated. Teardown functions can't modify the response, so there's no way to indicate if the operation actually failed.

[asteinlein]

You have error handlers for that.

[davidism]

Error handlers run before the teardown functions, so they wouldn't be able to catch the errors in this case.

[gdxn96]

I agree about the dislike of the pattern. However I'd much prefer to have a failsafe against lazy programmers, and have missing data associated with an error in the error log, rather than the data just be missing, requiring investigation. ~My two cents

Doing commit on teardown manually gave me the following issue
pallets/flask#984

Basically the manual way I did commit on teardown polluted my log file with

Unexpected exception [Errno 35] Resource temporarily unavailable

all over the place using gunicorn. Using this flag solves this issue for me, as obviously something fancy I haven't thought of is being implemented instead of my own teardown.

This was my teardown if interested

@app.teardown_request       
def teardown_request(exception):      
    if exception:     
        app.logger.error(exception)       
    try:      
        db.session.commit()       
    except:       
        db.session.rollback()     
    finally:      
        db.session.close()        
        db.session.remove()     

Sorry if this comment has no place on this thread, new enough to github 👍

[justanr]

@asteinlein I'd like to revisit the second part of my comment. Some of my opinions have changed and I also think it was misunderstood.

I understand the desire to not do db.commit() in every view. But that's already the wrong place, in my opinion, to handle it. The view should should serve as just a translation device between HTTP (or rather WSGI) and your application. For a small application, it's whatever but as an app grows, it'll begin buckling under pressure from this design.

I prefer to silo database access into repositories that handle all persistence concerns for me. This accomplishes two things:

1. When I test, I can provide an in memory version of the repository so I don't need to mock out the SQLA queries (which is terrible to do).
2. I have a place to put common queries. You can argue that on the model is the place to put these but I've always found this awkward.

Fixing the proliferation of finder methods is still there but if you're interested, we can take this off channel (@just_anr on twitter).

I said all that to say this: persistence is tricky and COMMIT_ON_TEARDOWN is unreliable at best. You should be handling commits, rollbacks and failures in your code. FSQLA probably shouldn't be making those choices for us. COMMIT_ON_TEARDOWN was a good idea, but how you handle a failed insert will change based on the data being inserted and FSQLA can't automate that for you. At least not easily.

[fuhrysteve]

My opinions have also changed since I was interested in this topic.

I agree with @justanr. COMMIT_ON_TEARDOWN encourages bad design that is difficult to test. Rather than explain in detail why I think this, I'll show an example and counter example.

Bad design:

from .models import Widget

@app.route('/widgets/<int:widget_id>')
def update_widgets(widget_id):
    widget = db.session.query(Widget).get_or_404(widget_id)
    widget.description = request.form['description']
    db.session.commit()
    widget_json = serialize_this_somehow(widget)
    return jsonify(data=widget_json)

Better design:

from .models import Widget

@app.route('/widgets/<int:widget_id>')
def update_widgets(widget_id):
    widget_json = widget_updater(widget_id, description=request.form['description'])
    return jsonify(data=widget_json)

def widget_updater(widget_id, description, session=None):
    session = session or db.session
    widget = db.session.query(Widget).get(widget_id)
    widget.description = description
    session.commit()
    return serialize_this_somehow(widget)

Now with very little adjustment, you can write a unit test for widget_updater that executes in 1 or 2ms instead of a 500ms integration test that requires a flask client and a database to run.

The point is, sessions and transactions shouldn't be encouraged to be synonymous with a web request. Those decisions should be made elsewhere.

[agronholm]

This is not an easy problem to solve in any elegant fashion. Basically when the request ends, any typical web app will want to do the following:

• commit the transaction
• render a template or serialize data
• return a response

If this is done in that very order, the session by default expires all objects, forcing a reload from the database when templating or serialization is done. If the order of the two actions is reversed, it will require an explicit db.session.flush() or else you may end up serializing transient objects that don't even have their defaults applied or the primary keys are missing.

How have others solved this problem?

[mliker]

Hi,

while I like the approach for better desing @fuhrysteve has outlined, it seems to me that this does not help with the scenario where the response is not processed by the client hence I have commited something that in the end did not happen.

Only after the client has accepted the answer would I like to commit a transaction involved.

How does one approach that?

[agronholm]

Does any existing web framework provide such guarantees?

[mliker]

Not that I am aware of, but I am no web developer hence my scope is limited to flask where I am building a simple REST service. However my background is in telecom and finance where you simply can not afford things like that.

One thing that I am thinking of right now is whether there is an ability to catch errors during sending of a response?

[agronholm]

There is no way to guarantee that the client has received a response, since according to the HTTP protocol the client does not send any acknowledgement for the response it received. The only clue would be when the client sends the TCP level ack packets for the response but I am not aware of any way to detect that on the application (framework) level.

[agronholm]

Besides, usually the framework sits behind a reverse proxy, so relying on TCP level ACKs won't work anyway.

[agronholm]

@mliker Are you suggesting that commit be done only after the response has been sent?

[mliker]

@agronholm well, I was imagining it could be wrapped in a custom context manager where upon a successful response the context manager would do commit. However I guess the rerun just ends the execution of a function.

However I am starting to feel that I might be over-engineering this.

[agronholm]

@gdxn96 I looked at your teardown function and it has a few problems:

1. I really don't think explicitly logging the exception should be done here, but left to other parts of the framework
2. The .remove() method also closes the session, making the .close() call redundant
3. Closing the session automatically rolls back the transaction (if needed for the engine), so I would consider the .rollback() redundant too

[agronholm]

Oh, and I forgot the most important part: your teardown function tries to commit even if the application raised an exception. Do you really want that?

[davidism]

Unfortunately missed the window to deprecate this in 2.4, so this will be deprecated in 3.0 and removed in 3.1. I'll release 2.4.3 with the deprecation warning as well so it gets exposed sooner.