Pass around the reactor explicitly #3385
Conversation
synapse/handlers/auth.py
Outdated
| @@ -425,12 +427,12 @@ def _check_msisdn(self, authdict, _): | |||
|
|
|||
| @defer.inlineCallbacks | |||
| def _check_dummy_auth(self, authdict, _): | |||
| yield run_on_reactor() | |||
| yield run_on_reactor(self.hs.get_clock()) | |||
There was a problem hiding this comment.
frankly I think it would be easier to bin these than to fix them. I think they've been cargo-culted. don't mind though.
There was a problem hiding this comment.
They do seem a bit weird to me, to be honest. I don't really see any benefit to putting things back on the reactor thread...
There was a problem hiding this comment.
the reasons for it now historical: back in the day (before various other things got fixed) you would get better stacktraces from deferreds which failed asynchronously than those that failed synchronously.
synapse/util/async.py
Outdated
|
|
||
|
|
||
| def run_on_reactor(): | ||
| def run_on_reactor(clock=None): |
synapse/util/async.py
Outdated
| @@ -404,7 +399,7 @@ class DeferredTimeoutError(Exception): | |||
| """ | |||
|
|
|||
|
|
|||
| def add_timeout_to_deferred(deferred, timeout, on_timeout_cancel=None): | |||
| def add_timeout_to_deferred(deferred, timeout, on_timeout_cancel=None, reactor=None): | |||
There was a problem hiding this comment.
looks like this will explode badly if called without a reactor param? might as well remove the default value
| return make_deferred_yieldable(threads.deferToThread(_do_hash)) | ||
| return make_deferred_yieldable( | ||
| threads.deferToThreadPool( | ||
| self.hs.get_reactor(), self.hs.get_reactor().getThreadPool(), _do_hash |
| @@ -858,7 +860,11 @@ def _do_hash(): | |||
| return bcrypt.hashpw(password.encode('utf8') + self.hs.config.password_pepper, | |||
| bcrypt.gensalt(self.bcrypt_rounds)) | |||
|
|
|||
| return make_deferred_yieldable(threads.deferToThread(_do_hash)) | |||
| return make_deferred_yieldable( | |||
| threads.deferToThreadPool( | |||
There was a problem hiding this comment.
why are we now using a threadpool here, ooi? (and what else does that threadpool do?)
There was a problem hiding this comment.
deferToThread implicitly uses the global reactor's threadpool -- so, this technically doesn't change behaviour, other than telling it explicitly what reactor (and what threadpool) we want to use. If we wished, we could add another threadpool explicitly for auth, here, rather than use the global one.
| @@ -37,13 +37,15 @@ class MediaStorage(object): | |||
| """Responsible for storing/fetching files from local sources. | |||
|
|
|||
| Args: | |||
| hs (Homeserver) | |||
| ratelimit, extra_users): | ||
| """Send event to be handled on the master | ||
|
|
||
| Args: | ||
| clock (Clock) |
There was a problem hiding this comment.
it would be nice to try and get the types for these right, to help editors implement clickthrough etc. I believe this is a synapse.util.Clock ?
| @@ -32,20 +32,15 @@ | |||
| logger = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| @defer.inlineCallbacks | |||
| def sleep(seconds): | |||
There was a problem hiding this comment.
I think you've missed a call to this in background_updates.py.
| @@ -34,9 +34,11 @@ class BackgroundFileConsumer(object): | |||
| # And resume once the size of the queue is less than this | |||
| _RESUME_ON_QUEUE_SIZE = 2 | |||
|
|
|||
| def __init__(self, file_obj): | |||
| def __init__(self, file_obj, reactor): | |||
| @@ -58,6 +58,7 @@ | |||
|
|
|||
| class MediaRepository(object): | |||
| def __init__(self, hs): | |||
| self.hs = hs | |||
There was a problem hiding this comment.
this seems to be a bit redundant, but <shrug>
|
@richvdh This branch is now free of the run_on_reactor changes. |
| @@ -186,6 +190,9 @@ def setup(self): | |||
| self.datastore = DataStore(self.get_db_conn(), self) | |||
| logger.info("Finished setting up.") | |||
|
|
|||
| def get_reactor(self): | |||
There was a problem hiding this comment.
I really wouldn't mind if this had a docstring; "Returns the twisted reactor in use by this HomeServer" or something?
This makes things easier to test down the road.