-
Notifications
You must be signed in to change notification settings - Fork 14
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
Prevent double-decode of url param. See #9 and #10 #11
Conversation
Do not decode url param a second time before converting. Causes normalization to be more painful than it needs to be, and the url is already decoded at this point. Normalization is necessary because cURL does not do it itself.
…rent hardcoded value of /tmp/node-image-magick
…cleaned up after a run
…rom being in one directory
…ting paths to the destination file, to allow for re-using converted images
…vent collisions at scale
…ent downloading a source file multiple times.
…e download. result is a 1-2 order of magnitude performance increase.
…d re-render it to the user in the future when more requests for it come in
what a pity that u put everything into once pull request. will merge it now |
@@ -138,7 +138,7 @@ RequestHandler.prototype._afterResize = function(err, path, res, callback) { | |||
res.contentType(path.replace(/\?(.*)/, "")) | |||
res.sendfile(path, { maxAge: oneYear }, function() { | |||
self.logger.log('File sent') | |||
self.deleteTempfile(path) | |||
// self.deleteTempfile(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's up with this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it so the converted file is cached and served if a request comes in to transform the file the same way, to the same size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right. now i see :) didn't notice that you pass the actual url to that method!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi: we just had a major fuckup as the hdds of our ec2 instances were out of space. I will add an options, which enables one to keep the downloaded files. also i will add an option for a limit of max used tmp space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, sorry to hear that. i threw a terabyte at ours so disk space isn't an issue, but the load we're seeing is causing a lot of nodejs restarts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do u mean by "causing a lot of nodejs restarts" ? we have an haproxy in front of our instances and are queueing requests if too many are approaching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's due to memory usage in the node process. We're also using haproxy for request queueing
thanks a lot :) nice work |
Do not decode url param a second time before converting. Causes normalization to be more painful than it needs to be, and the url is already decoded at this point.
Normalization is necessary because cURL does not do it itself.