Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Commit

Permalink
Fix concurrent modification errors in pusher metrics (#7106)
Browse files Browse the repository at this point in the history
add a lock to try to make this metric actually work
  • Loading branch information
richvdh committed Mar 19, 2020
1 parent 8c75667 commit e913823
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 9 deletions.
1 change: 1 addition & 0 deletions changelog.d/7106.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add prometheus metrics for the number of active pushers.
27 changes: 18 additions & 9 deletions synapse/push/pusherpool.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import logging
from collections import defaultdict
from threading import Lock
from typing import Dict, Tuple, Union

from twisted.internet import defer
Expand Down Expand Up @@ -56,12 +57,17 @@ def __init__(self, _hs):
# map from user id to app_id:pushkey to pusher
self.pushers = {} # type: Dict[str, Dict[str, Union[HttpPusher, EmailPusher]]]

# a lock for the pushers dict, since `count_pushers` is called from an different
# and we otherwise get concurrent modification errors
self._pushers_lock = Lock()

def count_pushers():
results = defaultdict(int) # type: Dict[Tuple[str, str], int]
for pushers in self.pushers.values():
for pusher in pushers.values():
k = (type(pusher).__name__, pusher.app_id)
results[k] += 1
with self._pushers_lock:
for pushers in self.pushers.values():
for pusher in pushers.values():
k = (type(pusher).__name__, pusher.app_id)
results[k] += 1
return results

LaterGauge(
Expand Down Expand Up @@ -293,11 +299,12 @@ def _start_pusher(self, pusherdict):
return

appid_pushkey = "%s:%s" % (pusherdict["app_id"], pusherdict["pushkey"])
byuser = self.pushers.setdefault(pusherdict["user_name"], {})

if appid_pushkey in byuser:
byuser[appid_pushkey].on_stop()
byuser[appid_pushkey] = p
with self._pushers_lock:
byuser = self.pushers.setdefault(pusherdict["user_name"], {})
if appid_pushkey in byuser:
byuser[appid_pushkey].on_stop()
byuser[appid_pushkey] = p

# Check if there *may* be push to process. We do this as this check is a
# lot cheaper to do than actually fetching the exact rows we need to
Expand Down Expand Up @@ -326,7 +333,9 @@ def remove_pusher(self, app_id, pushkey, user_id):
if appid_pushkey in byuser:
logger.info("Stopping pusher %s / %s", user_id, appid_pushkey)
byuser[appid_pushkey].on_stop()
del byuser[appid_pushkey]
with self._pushers_lock:
del byuser[appid_pushkey]

yield self.store.delete_pusher_by_app_id_pushkey_user_id(
app_id, pushkey, user_id
)

0 comments on commit e913823

Please sign in to comment.