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

Jump to bottom

models_committed signal triggered too early with nested transactions #645

Closed
michamos opened this issue Oct 10, 2018 · 4 comments · May be fixed by inspirehep/flask-sqlalchemy#1
Closed

models_committed signal triggered too early with nested transactions #645

michamos opened this issue Oct 10, 2018 · 4 comments · May be fixed by inspirehep/flask-sqlalchemy#1

Comments

@michamos
Copy link

michamos commented Oct 10, 2018 •
edited
Loading

I would like to be able to determine when changes have been committed to the db so that a query from a different worker will see the new data.

It looks like I could use the models_committed signal, but it doesn't seem to work properly when using SAVEPOINT transactions. In that case, the signal gets sent on the commit() of the nested transaction, rather than the real commit of the full transaction to the database.

This can be seen on the the following example:

from flask import Flask
from flask_sqlalchemy import SQLAlchemy, models_committed


app = Flask(__name__)
app.config['SQLALCHEMY_DATABASE_URI'] = 'sqlite:////tmp/test.db'
app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = True
db = SQLAlchemy(app)

class User(db.Model):
    id = db.Column(db.Integer, primary_key=True)
    username = db.Column(db.String(80), unique=True, nullable=False)
    email = db.Column(db.String(120), unique=True, nullable=False)


@models_committed.connect
def committed(*args, **kwargs):
    print('committed signal sent')


db.create_all()

with db.session.begin_nested():
    print('inside nested')
    admin = User(username='admin', email='[email protected]')
    db.session.add(admin)
    print('added admin')
print('outside nested')
db.session.commit()

The behavior I would expect is that the signal gets sent when doing the commit() on the last line, but instead the output is:

inside nested
added admin
committed signal sent
outside nested

due to the implicit commit() when exiting the context manager.

One solution would be to modify https://github.com/mitsuhiko/flask-sqlalchemy/blob/50944e77522d4aa005fc3c833b5a2042280686d3/flask_sqlalchemy/__init__.py#L212 in order to detect whether the current transaction is nested, and don't do anything in that case.

Is there any other way to reliably detect when a transaction has been committed?
@michamos
Copy link
Author

michamos commented Oct 11, 2018 •
edited
Loading

A simple solution is to replace the after_commit SQLAlchemy ORM event by after_transaction_end:

This event differs from after_commit() in that it corresponds to all SessionTransaction objects in use, including those for nested transactions and subtransactions, and is always matched by a corresponding after_transaction_create() event.

I am confused by this description. It looks like the after_commit SQLAlchemy event should only be triggered on real commits of the full transaction, as opposed to after_transaction_end, that should also be sent for nested transactions.

Edit: After checking in more detail, I found out that unlike the after_commit event which is triggered twice (once at the end of the nested transaction, once at the end of the full transaction), the after_transaction_end is triggered three times: twice at the end of the nested transaction (first with transaction.nested == False and then right after with transaction.nested == True) and once at the end of the full transaction (with transaction.nested == False).

@ammirate ammirate mentioned this issue Oct 11, 2018
Closed
michamos added a commit to michamos/flask-sqlalchemy that referenced this issue Oct 15, 2018
@michamos
correctly handle signals in nested transactions
c34ad3b 
closes pallets-eco#645
michamos added a commit to michamos/flask-sqlalchemy that referenced this issue Oct 15, 2018
@michamos
correctly handle signals in nested transactions
61f6c09 
closes pallets-eco#645
@michamos michamos mentioned this issue Oct 15, 2018
Closed
michamos added a commit to michamos/flask-sqlalchemy that referenced this issue Oct 16, 2018
@michamos
correctly handle signals in nested transactions
57233f4 
closes pallets-eco#645
This was referenced Nov 16, 2018
Closed
Open
monaawi pushed a commit to inspirehep/flask-sqlalchemy that referenced this issue Nov 16, 2018
@michamos @monaawi
correctly handle signals in nested transactions
63732c9 
closes pallets-eco#645
monaawi pushed a commit to inspirehep/flask-sqlalchemy that referenced this issue Nov 16, 2018
@michamos @monaawi
correctly handle signals in nested transactions
0b74127 
closes pallets-eco#645
monaawi pushed a commit to inspirehep/flask-sqlalchemy that referenced this issue Nov 27, 2018
@michamos @monaawi
correctly handle signals in nested transactions
35ab75e 
closes pallets-eco#645
@ticosax
Copy link

ticosax commented Jan 21, 2019

SQLAlchemy exposes after_transaction_end.
Wouldn't it be better to rely on this signal to serve your use case ?

@rsyring rsyring added this to the Unknown milestone Mar 9, 2019
@michamos
Copy link
Author

michamos commented Jun 6, 2019

That doesn't work, as that signal is emitted for every transaction end, not only the outer one. The solution adopted in #646 is to check explicitly if the transaction is nested or a subtransaction in all the different handlers.

michamos added a commit to inspirehep/flask-sqlalchemy that referenced this issue Jun 6, 2019
@michamos
correctly handle signals in nested transactions
71abd94 
closes pallets-eco#645
drjova pushed a commit to inspirehep/flask-sqlalchemy that referenced this issue Jun 11, 2019
@michamos @drjova
correctly handle signals in nested transactions
a817ea2 
closes pallets-eco#645
@TTWShell TTWShell mentioned this issue May 18, 2022
Merged
@davidism
Copy link
Member

see close reason in associated PR #646

@davidism davidism removed this from the Unsure milestone Sep 18, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2022
MJedr pushed a commit to inspirehep/flask-sqlalchemy that referenced this issue Jul 11, 2023
@michamos @MJedr
correctly handle signals in nested transactions
d315f44 
closes pallets-eco#645
MJedr pushed a commit to inspirehep/flask-sqlalchemy that referenced this issue Jul 12, 2023
@michamos @MJedr
correctly handle signals in nested transactions
9e9b357 
closes pallets-eco#645
MJedr pushed a commit to inspirehep/flask-sqlalchemy that referenced this issue Jul 12, 2023
@michamos @MJedr
correctly handle signals in nested transactions
602df9b 
closes pallets-eco#645
MJedr pushed a commit to inspirehep/flask-sqlalchemy that referenced this issue Jul 12, 2023
@michamos @MJedr
correctly handle signals in nested transactions
8c6744f 
closes pallets-eco#645
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
4 participants
@rsyring @ticosax @davidism @michamos