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

Port synapse.replication.tcp to async/await #6666

Merged
merged 5 commits into from
Jan 16, 2020

Conversation

erikjohnston
Copy link
Member

No description provided.

@erikjohnston erikjohnston requested a review from a team January 10, 2020 10:15
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so i think this works, however...

the on_foo methods, and subscribe_to_stream, are called from on_FOO methods, which are documented to return Deferreds.

I think you're getting away with it because this means that they are returning a Coroutine, and the thing calling them does a run_as_background_process on them so is happy with that... but to be correct that code path needs some documentation updates at least.

@erikjohnston
Copy link
Member Author

Yeah, agreed. I could have just documented the functions as returning Awaitable, but instead I've just gone and marked them all as async, as that felt saner.

@erikjohnston erikjohnston requested a review from richvdh January 15, 2020 16:16
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm other than the below

@@ -376,7 +372,7 @@ def connectionLost(self, reason):

self.on_connection_closed()

def on_connection_closed(self):
async def on_connection_closed(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one isn't called the same way

"""Called when we get new position data."""
raise NotImplementedError()

@abc.abstractmethod
def on_sync(self, data):
async def on_sync(self, data):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the implementations of this don't seem to be async (and it's not awaited where it is called).

@erikjohnston
Copy link
Member Author

Honestly its like I can't even tell the difference between ALL CAPS and lower case...

@erikjohnston erikjohnston merged commit 48c3a96 into develop Jan 16, 2020
@erikjohnston erikjohnston deleted the erikj/async_await_replication branch February 5, 2020 17:35
@clokep clokep mentioned this pull request Jul 30, 2020
48 tasks
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit '48c3a9688':
  Port synapse.replication.tcp to async/await (#6666)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants