jazzband / sorl-thumbnail

Thumbnails for Django
https://sorl-thumbnail.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1.74k stars 497 forks source link

THUMBNAIL_DUMMY only active when IOError occurs #638

Open jayvdb opened 4 years ago

jayvdb commented 4 years ago

For the same scenario as in https://github.com/jazzband/sorl-thumbnail/issues/637 , THUMBNAIL_DUMMY doesnt work.

Copying a snipped of that backtrace:

  File "/usr/lib/python3.8/site-packages/sorl/thumbnail/base.py", line 108, in get_thumbnail
    source_image = default.engine.get_image(source)
  File "/usr/lib/python3.8/site-packages/sorl/thumbnail/engines/pil_engine.py", line 72, in get_image
    buffer = BytesIO(source.read())
  File "/usr/lib/python3.8/site-packages/sorl/thumbnail/images.py", line 162, in read
    f = self.storage.open(self.name)

Here is what sorl/thumbnail/base.py looks like around line 108 (line numbers slightly off due to injected debugging).

        # We have to check exists() because the Storage backend does not
        # overwrite in some implementations.
        if settings.THUMBNAIL_FORCE_OVERWRITE or not thumbnail.exists():
            try:
                source_image = default.engine.get_image(source)
            except IOError as e:
                logger.exception(e)
                if settings.THUMBNAIL_DUMMY:
                    return DummyImageFile(geometry_string)
                else:

If the thumbnail does not yet exist, the storage will be used first, and the dummy only used if there was an IOError. Even if that code was being triggered, the dummy image isnt cached, so the same timeout would happen for the next time the same image is requested.

In any case, that code path is never reached because the MRO of these exceptions from the storage doesnt include IOError. Here is the timeout error MRO:

jayvdb commented 4 years ago

b2d90acd is where the catching of IOError was introduced, in 2012.

While the semantics of https://docs.djangoproject.com/en/3.0/ref/files/storage/ have an obvious link to file IO, nothing there or at https://docs.djangoproject.com/en/3.0/howto/custom-file-storage/ dictates what types of exceptions should be raised.

I've looked through a few storage backends and havent found any which intentionally catch exceptions in order to raise IOError. Consequently I think this exception handling in sorl.thumbnail should catch any error.

jayvdb commented 4 years ago

639 only catches the error. It doesnt address the problem of the dummy only being used after the exception has occurred - in the event of the image fetch causing a timeout, that ordering is a bit annoying.

However if the dummy should only be used as a fallback, I could ask the backends to allow the timeout to be configurable to lessen the pain.

camilonova commented 3 years ago

@jayvdb can you update the status of this issue?

jayvdb commented 3 years ago

No significant change.

The title of this issue could now be "THUMBNAIL_DUMMY only active when an storage exception occurs"

The main problem is that storage are usually remote, so they usually have timeouts, and there is no caching of which storages or media are broken, so the dummy thumbnail wont help much towards making pages any more usable - pages will still take a long time to load, especially if the page has lots of media on it, with the small benefit that a placeholder image is shown for the images which are broken.

Unfortunately there isnt a global STORAGES, like there is DATABASES, so we'd need our own tracking objects to indicate which storages are offline/broken/etc.

The main scenario I am interested in is dev servers where the storage server is slow (especially startup), or offline, and I would be quite happy with a ALWAYS_USE_DUMMY_THUMBAIL so that the local fake s3 server can be disabled unless I am doing dev work which concerns the media.