Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve cover image performance #3580

Merged
merged 6 commits into from
Nov 2, 2024

Conversation

mikiher
Copy link
Contributor

@mikiher mikiher commented Nov 2, 2024

Following the experiments described here, I made the following changes to cover image GET requests (I will do the same for author images in separate PR - wanted to focus on covers here):

  • Removed authentication and user deserialization (req.user) from cover requests
  • Optimized LibraryItemController.getCover so that no database access is made if the resized image is already in the covers disk cache
  • Removed the token parameter from cover urls on the web client

This reduces the average completion time for cover requests by a factor of ~10 when acessing the server through https, and minimizes concurrent access to the database.

server/Server.js Dismissed Show dismissed Hide dismissed
server/Server.js Dismissed Show dismissed Hide dismissed
server/Server.js Dismissed Show dismissed Hide dismissed
server/Server.js Dismissed Show dismissed Hide dismissed
@mikiher mikiher marked this pull request as ready for review November 2, 2024 13:56
@advplyr
Copy link
Owner

advplyr commented Nov 2, 2024

Thanks!

@advplyr advplyr merged commit 654b1d6 into advplyr:master Nov 2, 2024
5 checks passed
@mikiher mikiher deleted the cover-image-performance branch November 18, 2024 07:03
Comment on lines +30 to +32
authNotNeeded(req) {
return req.method === 'GET' && this.ignorePattern.test(req.originalUrl)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this potentially leak the content of the library if you somehow get your hand on the item IDs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does. We discussed this and concluded that the security risk is not very high in this case.

The situation up until now was much worse in my opinion - we sent authentication token as a url query parameter in cover and author img tags, and now we don't do that any more.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mikiher It's a lot of work but usually people solve this problem using HMAC signatures that is provided in the GET parameters of the URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I read about some of these solutions a while back. It's not worth the investment, IMO. This isn't a DoD app 🙂

@watsonbox
Copy link

Excellent performance investigation and improvements, thanks a lot 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants