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

Fix async/await calls for broken media providers. #8027

Merged
merged 1 commit into from
Aug 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/8027.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Convert various parts of the codebase to async/await.
23 changes: 5 additions & 18 deletions synapse/rest/media/v1/media_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import contextlib
import inspect
import logging
import os
import shutil
Expand All @@ -30,7 +29,7 @@
if TYPE_CHECKING:
from synapse.server import HomeServer

from .storage_provider import StorageProvider
from .storage_provider import StorageProviderWrapper

logger = logging.getLogger(__name__)

Expand All @@ -50,7 +49,7 @@ def __init__(
hs: "HomeServer",
local_media_directory: str,
filepaths: MediaFilePaths,
storage_providers: Sequence["StorageProvider"],
storage_providers: Sequence["StorageProviderWrapper"],
):
self.hs = hs
self.local_media_directory = local_media_directory
Expand Down Expand Up @@ -115,11 +114,7 @@ def store_into_file(self, file_info: FileInfo):

async def finish():
for provider in self.storage_providers:
# store_file is supposed to return an Awaitable, but guard
# against improper implementations.
result = provider.store_file(path, file_info)
if inspect.isawaitable(result):
await result
await provider.store_file(path, file_info)

finished_called[0] = True

Expand Down Expand Up @@ -153,11 +148,7 @@ async def fetch_media(self, file_info: FileInfo) -> Optional[Responder]:
return FileResponder(open(local_path, "rb"))

for provider in self.storage_providers:
res = provider.fetch(path, file_info) # type: Any
# Fetch is supposed to return an Awaitable[Responder], but guard
# against improper implementations.
if inspect.isawaitable(res):
res = await res
res = await provider.fetch(path, file_info) # type: Any
if res:
logger.debug("Streaming %s from %s", path, provider)
return res
Expand All @@ -184,11 +175,7 @@ async def ensure_media_is_in_local_cache(self, file_info: FileInfo) -> str:
os.makedirs(dirname)

for provider in self.storage_providers:
res = provider.fetch(path, file_info) # type: Any
# Fetch is supposed to return an Awaitable[Responder], but guard
# against improper implementations.
if inspect.isawaitable(res):
res = await res
res = await provider.fetch(path, file_info) # type: Any
if res:
with res:
consumer = BackgroundFileConsumer(
Expand Down
19 changes: 15 additions & 4 deletions synapse/rest/media/v1/storage_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import inspect
import logging
import os
import shutil
Expand Down Expand Up @@ -88,20 +89,30 @@ async def store_file(self, path, file_info):
return None

if self.store_synchronous:
return await self.backend.store_file(path, file_info)
# store_file is supposed to return an Awaitable, but guard
# against improper implementations.
result = self.backend.store_file(path, file_info)
if inspect.isawaitable(result):
return await result
else:
# TODO: Handle errors.
def store():
async def store():
try:
return self.backend.store_file(path, file_info)
result = self.backend.store_file(path, file_info)
if inspect.isawaitable(result):
return await result
Comment on lines +99 to +103
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't 100% necessary, but I think without it we're going to end up with "coroutine wasn't awaited on" in the logs.

Copy link
Member

Choose a reason for hiding this comment

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

coroutine wasn't awaited on means that it won't get run at all, aiui.

I think in this case it was fine because the coroutine was returned to run_in_background which then awaits

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah fair enough, should I revert this then?

Copy link
Member

Choose a reason for hiding this comment

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

Nah, tbh i think consistency here ftw

except Exception:
logger.exception("Error storing file")

run_in_background(store)
return None

async def fetch(self, path, file_info):
return await self.backend.fetch(path, file_info)
# store_file is supposed to return an Awaitable, but guard
# against improper implementations.
result = self.backend.fetch(path, file_info)
if inspect.isawaitable(result):
return await result


class FileStorageProviderBackend(StorageProvider):
Expand Down