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

fix race condiftion in calling initialise_reserved_users #4081

Merged
merged 7 commits into from Oct 25, 2018

Conversation

2 participants
@neilisfragile
Copy link
Contributor

neilisfragile commented Oct 23, 2018

No description provided.

@neilisfragile neilisfragile requested a review from matrix-org/synapse-core Oct 23, 2018

Show resolved Hide resolved synapse/storage/monthly_active_users.py Outdated
Show resolved Hide resolved synapse/storage/monthly_active_users.py
Show resolved Hide resolved synapse/storage/monthly_active_users.py Outdated
Show resolved Hide resolved synapse/storage/monthly_active_users.py Outdated
Show resolved Hide resolved synapse/storage/monthly_active_users.py Outdated
Show resolved Hide resolved synapse/storage/monthly_active_users.py Outdated
Show resolved Hide resolved synapse/storage/monthly_active_users.py
Show resolved Hide resolved synapse/storage/monthly_active_users.py
Show resolved Hide resolved synapse/storage/monthly_active_users.py Outdated
Show resolved Hide resolved synapse/storage/registration.py
Show resolved Hide resolved changelog.d/4081.bugfix Outdated
@richvdh
Copy link
Member

richvdh left a comment

lgtm other than the below.

user_id (str): user to add/update
"""
is_insert = yield self.runInteraction(
"upsert_monthly_active_user", self.upsert_monthly_active_user_txn,
user_id
)
# Considered pushing cache invalidation down into txn method, but

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 25, 2018

Member

sorry, this isn't terribly easy to read; and I think it's actually more important to put something in the _txn method. How about getting rid of this, and instead, putting the following in the docstring of the _txn method:

Note that, after calling this method, it will generally be necessary to invalidate the caches on user_last_seen_monthly_active and get_monthly_active_count. We can't do that here, because we are running in a database thread rather than the main thread, and we can't call txn.call_after because txn may not be a LoggingTransaction.

This comment has been minimized.

Copy link
@neilisfragile

neilisfragile Oct 25, 2018

Author Contributor

wfm

@richvdh richvdh assigned neilisfragile and unassigned richvdh Oct 25, 2018


def upsert_monthly_active_user_txn(self, txn, user_id):
"""Updates or inserts monthly active user member
Note that, after calling this method, it will generally be necessary

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 25, 2018

Member

Suggest an extra blank line here:

Suggested change
Note that, after calling this method, it will generally be necessary
Note that, after calling this method, it will generally be necessary

@neilisfragile neilisfragile merged commit 95ad128 into develop Oct 25, 2018

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@neilisfragile neilisfragile deleted the neilj/fix_mau_init branch Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.