search

matrix-org / synapse-s3-storage-provider

Synapse storage provider to fetch and store media in Amazon S3
Apache License 2.0
132 stars 36 forks source link

Use the right config section when starting the module #62

Closed babolivier closed 3 years ago

patrickstump commented 3 years ago

@babolivier , I know this is after the fact, but is there value to adding a version check since this popped up in v1.45.0? That is, if someone is running < v1.45.0 would it be required to use the previous call of self.cache_directory = hs.config.media_store_path ?

babolivier commented 3 years ago

@babolivier , I know this is after the fact, but is there value to adding a version check since this popped up in v1.45.0? That is, if someone is running < v1.45.0 would it be required to use the previous call of self.cache_directory = hs.config.media_store_path ?

No, with Synapse < v1.45.0, reading hs.config.media.media_store_path still works.

anoadragon453 commented 3 years ago

@patrickstump For context we had a magic method which translated hs.config.media_store_path to hs.config.media.media_store_path. The method has now been removed, as it was a noticeable performance loss for minor developer convenience. But using the latter always worked :slightly_smiling_face:

richvdh commented 3 years ago

To be clear, is this fixing something that makes s3-storage-provider not work with newer versions of synapse? If so, what were the symptoms of the failure? Some sort of exception?

babolivier commented 3 years ago

To be clear, is this fixing something that makes s3-storage-provider not work with newer versions of synapse? If so, what were the symptoms of the failure? Some sort of exception?

Yes, because media storage providers apparently get passed the Synapse config directly, and v1.45.0 removed the magic that translated hs.config.media_store_path into hs.config.media.media_store_path. This module was reading from hs.config.media_store_path, which raised an AttributeError and would prevent either "just" the media repository workers or the whole homeserver (if not using workers) from starting.

We should have caught this in https://github.com/matrix-org/synapse/pull/10985#issuecomment-936056980, but forgot about the particular case of media storage providers.

richvdh commented 3 years ago

ok, thanks Brendan.

As a reminder, it's very helpful to include an example of the error message, so that other people seeing the same error can search for it and find the fix.

babolivier commented 3 years ago

Ah good point. Here's the full stacktrace:

Traceback (most recent call last):
  File "/home/synapse/src/synapse/app/_base.py", line 196, in wrapper
    await cb(*args, **kwargs)
  File "/home/synapse/src/synapse/app/_base.py", line 387, in start
    hs.start_listening()
  File "/home/synapse/src/synapse/app/generic_worker.py", line 389, in start_listening
    self._listen_http(listener)
  File "/home/synapse/src/synapse/app/generic_worker.py", line 325, in _listen_http
    media_repo = self.get_media_repository_resource()
  File "/home/synapse/src/synapse/server.py", line 185, in _get
    dep = builder(self)
  File "/home/synapse/src/synapse/server.py", line 615, in get_media_repository_resource
    return MediaRepositoryResource(self)
  File "/home/synapse/src/synapse/rest/media/v1/media_repository.py", line 986, in __init__
    media_repo = hs.get_media_repository()
  File "/home/synapse/src/synapse/server.py", line 185, in _get
    dep = builder(self)
  File "/home/synapse/src/synapse/server.py", line 619, in get_media_repository
    return MediaRepository(self)
  File "/home/synapse/src/synapse/rest/media/v1/media_repository.py", line 108, in __init__
    backend = clz(hs, provider_config)
  File "/home/synapse/synapse-s3-storage-provider/s3_storage_provider.py", line 63, in __init__
    self.cache_directory = hs.config.media_store_path
AttributeError: 'HomeServerConfig' object has no attribute 'media_store_path'
HarHarLinks commented 3 years ago

@babolivier , I know this is after the fact, but is there value to adding a version check since this popped up in v1.45.0? That is, if someone is running < v1.45.0 would it be required to use the previous call of self.cache_directory = hs.config.media_store_path ?

No, with Synapse < v1.45.0, reading hs.config.media.media_store_path still works.

I upgraded synapse-s3-storage-provider together with synapse 1.45.1 using pip install --force-reinstall, but had to downgrade back to 1.43 (https://github.com/matrix-org/synapse/issues/11049#issuecomment-948545163). Now can't upload media, error log:

2021-10-21 12:17:51,904 - synapse.http.server - 93 - ERROR - POST-7488- Failed handle request via 'UploadResource': <XForwardedForRequest at 0x7f3bfbc62e20 method='POST' uri='/_matrix/media/r0/upload' clientproto='HTTP/1.0' site='8008'>
Traceback (most recent call last):
  File "site-packages/synapse/http/server.py", line 258, in _async_render_wrapper
    callback_return = await self._async_render(request)
  File "site-packages/synapse/http/server.py", line 286, in _async_render
    callback_return = await raw_callback_return
  File "site-packages/synapse/rest/media/v1/upload_resource.py", line 93, in _async_render_POST
    content_uri = await self.media_repo.create_content(
  File "site-packages/synapse/rest/media/v1/media_repository.py", line 169, in create_content
    fname = await self.media_storage.store_file(content, file_info)
  File "site-packages/synapse/rest/media/v1/media_storage.py", line 81, in store_file
    await self.write_to_file(source, f)
  File "site-packages/synapse/rest/media/v1/media_storage.py", line 88, in write_to_file
    await defer_to_thread(self.reactor, _write_file_synchronously, source, output)
  File "site-packages/twisted/python/threadpool.py", line 238, in inContext
    result = inContext.theWork()  # type: ignore[attr-defined]
  File "site-packages/twisted/python/threadpool.py", line 254, in <lambda>
    inContext.theWork = lambda: context.call(  # type: ignore[attr-defined]
  File "site-packages/twisted/python/context.py", line 118, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "site-packages/twisted/python/context.py", line 83, in callWithContext
    return func(*args, **kw)
  File "site-packages/synapse/logging/context.py", line 902, in g
    return f(*args, **kwargs)
  File "site-packages/synapse/rest/media/v1/media_storage.py", line 298, in _write_file_synchronously
    source.seek(0)  # Ensure we read from the start of the file
ValueError: seek of closed file

So apparently this is not backwards compatible?

Also synapse.rest.media.v1._base - 256 - WARNING - GET-40201- Failed to write to consumer: <class 'Exception'> Consumer asked us to stop producing happens.

callahad commented 3 years ago

@HarHarLinks Can you please open a new issue, also if you could add the output of pip freeze and more log context around the error?