🐛 Fix invalid image URLs not being cached and causing timeouts #8986

aileen · 2017-09-07T09:36:23Z

swapped the usage of got for requests with the request util, which validates a URL before doing a request and resolving the Promises in image-size so it gets saved in the image-size cache.
Uses catch predicates instead of conditionals in getImageSizeFromUrl
Return NotFoundError if applicable in getImageSizeFromFilePath as the caller function cachedImageSizeFromUrl is differentiating those between this error and others.
Using ImageObject as a global var resulted in having the url property being the same for all requests coming in.
The logic that checked for an existing protocol (e. g. gravatar URLs) was overly complicated. Refactored it to be more simple.
Passing the correct value to fetchDimensionsFromBuffer as the population of imageObject.url happens there. These are used in our structured data and need to be full URLs (in case of locally stored files) or the original URL (in case of URLs missing the protocol)
Added two more debug logs in getCachedImageSizeFromUrl so it's logged when an image is added to the cache even tho it was returned as error.
We're always resolving the result in getCachedImageSizeFromUrl, so there's no need to return the values with a Promise.resolve(). The caller fn uses waits for the Promises to be fulfilled.

Todos:

investigate and refactor usage of Promise.resolve() in cached image size
Use predicate catches in image-size
Add NotFoundError handling to storage.read request, as the calling fn cachedImageSizeFromUrl is differentiating those errors
check if global var imageObject interferes with image size cache
find and improve unnecessary code (nice to have)
solve catch predicates issue see 🐛 Fix invalid image URLs not being cached and causing timeouts #8986 (comment)

aileen · 2017-09-08T01:43:05Z

@ErisDS one question I have on my mind is the handling of the different errors in cachedImageSizeFromUrl. Right now, we're not logging or producing the NotFoundErrors see here but everything else and I'm wondering why? Mostly the question if timeout errors should also be caught here, as they can be caused by a redirect which is indeed a bit slower now.

core/server/utils/image-size.js

+                message: err.message,
+                code: 'IMAGE_SIZE',
+                err: err,
+                context: {


aileen · 2017-09-08T03:57:27Z

There seems to be still some issues with the errors returned from the got, is the catch predicates in getCachedImageSizeFromUrl don't handle it properly. It works totally fine with errors returned from reading the storage.

So here's what's happening:

Any filtered catch (e. g. NotFoundError) works in getImageSizeFromUrl but then it seems like this error (at least it is not the got error anymore, but the NotFoundError we returned) gets caught by the 'catch all' predicate and that's what get's returned to getCachedImageSizeFromUrl in the end so the predicates there won't work. I can't explain why this happens. It works fine when I remove the 'catch all' predicate in getImageSizeFromUrl, but this is definitely not a solution!

ErisDS · 2017-09-08T08:37:52Z

@ErisDS one question I have on my mind is the handling of the different errors in cachedImageSizeFromUrl. Right now, we're not logging or producing the NotFoundErrors see here but everything else and I'm wondering why? Mostly the question if timeout errors should also be caught here, as they can be caused by a redirect which is indeed a bit slower now.

We don't do anything with not found errors because they are an expected case. Images will 404 over time. We don't want to fill up the logs, nothing went wrong :)

Any filtered catch (e. g. NotFoundError) works in getImageSizeFromUrl but then it seems like this error (at least it is not the got error anymore, but the NotFoundError we returned) gets caught by the 'catch all' predicate and that's what get's returned to getCachedImageSizeFromUrl in the end so the predicates there won't work. I can't explain why this happens. It works fine when I remove the 'catch all' predicate in getImageSizeFromUrl, but this is definitely not a solution!

This is expected behaviour as far as I know. If you catch an error in a predicate and then re-throw (reject) it, it will next get caught by the catch all. The way to deal with this is to check in the catch-all for any errors which are not already GhostErrors. If not, wrap them, else, just reject again.

aileen · 2017-09-08T09:04:52Z

We don't do anything with not found errors because they are an expected case. Images will 404 over time. We don't want to fill up the logs, nothing went wrong :)

I am sorry, I expressed myself wrong :( I was referring to logging the errors. My concern was only, that also timeout errors might now increase (because of the ability to follow redirects and they actually take a bit longer and might hit the timeout) and will therefore be logged. My question was, if we want them to be logged as well?

ErisDS · 2017-09-08T09:36:42Z

@AileenCGN sounds like you might be onto something - my advice would be to experiment with what feels right.

aileen · 2017-09-08T10:54:05Z

@ErisDS This PR is ready for review and merge 🎉

core/server/utils/image-size.js

@@ -143,6 +143,9 @@ getImageSizeFromUrl = function getImageSizeFromUrl(imagePath) {
            context: err.url || imagePath
        }));
    }).catch(function (err) {
+        if (err instanceof errors.NotFoundError || err instanceof errors.InternalServerError) {


core/server/utils/image-size.js

@@ -199,6 +202,9 @@ getImageSizeFromFilePath = function getImageSizeFromFilePath(imagePath) {
                }
            }));
        }).catch(function (err) {
+            if (err instanceof errors.NotFoundError || err instanceof errors.InternalServerError) {


ErisDS · 2017-09-11T13:01:19Z

Just checking: as a result of these changes, what now happens in the original error case I had with the old-nonsubdirectory local path?

aileen · 2017-09-11T13:10:01Z

Just checking: as a result of these changes, what now happens in the original error case I had with the old-nonsubdirectory local path?

This case will result in an invalid or missing URL error thrown here and will be caught here: https://github.com/TryGhost/Ghost/pull/8986/files#diff-6d3ba398735146e7c9d8c64ac57dbc5bR34

The cache issue is resolved as well, so a second request results in reading from the cache.

ErisDS · 2017-09-11T13:26:25Z

Awesome 👍

There are a couple of minor amends needed. Will review again first thing in the am :)

aileen · 2017-09-11T13:32:41Z

Summary of cases I successfully tested:

Local storage files
- Missing local image
- old non-subdirectory path
Image URL
- redirected image
- invalid image URL
- image not found (404)
- timeouts for image requests

Of course I tested positive cases as well ;)
For all of these cases, the cache works and the error handling does its job as well.

refs TryGhost#8868 - swapped the usage of `got` for requests with the request util

refs TryGhost#8868 - Uses catch predicates instead of conditionals in `getImageSizeFromUrl` - Return `NotFoundError` if applicable in `getImageSizeFromFilePath` as the caller function `cachedImageSizeFromUrl` is differentiating those between this error and others.

- Using `ImageObject` as a global var resulted in having the `url` property being the same for all requests coming in. - The logic that checked for an existing protocol (e. g. gravatar URLs) was overly complicated. Refactored it to be more simple. - Passing the correct value to `fetchDimensionsFromBuffer` as the population of `imageObject.url` happens there. These are used in our structured data and need to be full URLs (in case of locally stored files) or the original URL (in case of URLs missing the protocol) - Added two more debug logs in `getCachedImageSizeFromUrl` so it's logged when an image is added to the cache even tho it was returned as error.

We're always resolving the result in `getCachedImageSizeFromUrl`, so there's no need to return the values with a `Promise.resolve()`. The caller fn uses waits for the Promises to be fulfilled.

closes TryGhost/Support#361 - adds a guard that checks the image URL for `/assets/` in the beginning and passes a completed URL to the request util to try and fetch the image size - adds tests

aileen · 2017-09-12T10:32:02Z

@ErisDS fyi I left commits separate to make the review easier, but they should totally be squashed when being merged.

closes #8589, refs #8868 This PR ports most of the changes of #8892 and #8986 to improve the image-size util. - Swapped `request` with `got` in `getImageSizeFromUrl` fn. - wrapped `got` request library in its own `request.js` util that returns bluebird promises and validates URL before starting a request - Used catch predicates instead of conditionals in `getImageSizeFromUrl` - Returned `NotFoundError` if applicable as the caller function `cachedImageSizeFromUrl` is differentiating those between this error and others. - The logic that checked for an existing protocol (e. g. gravatar URLs) was overly complicated. Refactored it to be more simple. - We're always resolving the result in `getCachedImageSizeFromUrl`, so there's no need to return the values with a `Promise.resolve()`. The caller fn uses waits for the Promises to be fulfilled. - added and improved tests

aileen force-pushed the use-request-util-for-image-size branch 6 times, most recently from 74220cd to 7e6e944 Compare September 7, 2017 13:53

aileen force-pushed the use-request-util-for-image-size branch from 686706f to 77c2a1f Compare September 8, 2017 02:54

aileen commented Sep 8, 2017

View reviewed changes

core/server/utils/image-size.js Outdated

message: err.message,

code: 'IMAGE_SIZE',

err: err,

context: {

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

aileen changed the title ~~[WIP] 🐛 Fix invalid image URLs not being cached and causing timeouts~~ 🐛 Fix invalid image URLs not being cached and causing timeouts Sep 8, 2017

aileen assigned ErisDS Sep 8, 2017

ErisDS reviewed Sep 11, 2017

View reviewed changes

aileen force-pushed the use-request-util-for-image-size branch from 1a01fc0 to 1fa3dbe Compare September 11, 2017 13:27

aileen added 7 commits September 12, 2017 15:47

📐 Use request util in image-size

dee7d81

refs TryGhost#8868 - swapped the usage of `got` for requests with the request util

👀 Differentiate error codes between request and storage

e617f3c

🔥 Remove not needed Promise.resolve()

299f657

We're always resolving the result in `getCachedImageSizeFromUrl`, so there's no need to return the values with a `Promise.resolve()`. The caller fn uses waits for the Promises to be fulfilled.

☂️ Wrap already rejected predicate errors in catch all

5159c57

Use errorDetails instead of context

1ef465a

☂️ Support /assets/ image paths

f7afb8b

closes TryGhost/Support#361 - adds a guard that checks the image URL for `/assets/` in the beginning and passes a completed URL to the request util to try and fetch the image size - adds tests

aileen force-pushed the use-request-util-for-image-size branch from 1fa3dbe to f7afb8b Compare September 12, 2017 10:05

ErisDS merged commit a45a91c into TryGhost:master Sep 12, 2017

aileen mentioned this pull request Sep 13, 2017

🙈 Used got to handle requests for image-size #8953

Merged

aileen deleted the use-request-util-for-image-size branch October 13, 2017 08:30

ErisDS removed their assignment Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix invalid image URLs not being cached and causing timeouts #8986

🐛 Fix invalid image URLs not being cached and causing timeouts #8986

aileen commented Sep 7, 2017 •

edited

Loading

aileen commented Sep 8, 2017

This comment was marked as abuse.

This comment was marked as abuse.

aileen commented Sep 8, 2017

ErisDS commented Sep 8, 2017

aileen commented Sep 8, 2017

ErisDS commented Sep 8, 2017

aileen commented Sep 8, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

ErisDS commented Sep 11, 2017

aileen commented Sep 11, 2017

ErisDS commented Sep 11, 2017

aileen commented Sep 11, 2017

aileen commented Sep 12, 2017

🐛 Fix invalid image URLs not being cached and causing timeouts #8986

🐛 Fix invalid image URLs not being cached and causing timeouts #8986

Conversation

aileen commented Sep 7, 2017 • edited Loading

aileen commented Sep 8, 2017

This comment was marked as abuse.

This comment was marked as abuse.

aileen commented Sep 8, 2017

ErisDS commented Sep 8, 2017

aileen commented Sep 8, 2017

ErisDS commented Sep 8, 2017

aileen commented Sep 8, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

ErisDS commented Sep 11, 2017

aileen commented Sep 11, 2017

ErisDS commented Sep 11, 2017

aileen commented Sep 11, 2017

aileen commented Sep 12, 2017

aileen commented Sep 7, 2017 •

edited

Loading