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

Do not call the callback function twice when image size detection failed #25

Merged
merged 2 commits into from
Sep 11, 2014
Merged

Conversation

shinnn
Copy link
Contributor

@shinnn shinnn commented Jul 9, 2014

Currently image-size calls the callback function twice when the function throws an error.
This causes a confusion. When the callback function itself fails though size detection succeed, image-size tries to run the callback again.

For example, this code shows the log twice, unfortunately.

var sizeOf = require('image-size');

sizeOf('img.jpg', function() {
  console.log('This message should be shown only once!');
  throw new Error();
})
> This message should be shown only once!
> This message should be shown only once!

And besides, it causes a problem on asynchronous iteration using async or each-async etc.

For example,

var async = require('async');
var sizeOf = require('image-size');

async.each(imagePathArray, function(imagePath, i, next) {
  imageSize(imagePath, next);
}, function() {
  assert.equal(0, 100); // it throws an error
});

When we run the code above, we expect an error message such as Assertion errror: 0 != 100.
However, the code throws an iteration error like Callback already called. instead of assertion error, because the callback function is called twice.

This PR fixes these problem. When the callback function is called, image-size will finish immediately.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) when pulling fd9a799 on shinnn:master into c2a859f on netroy:master.

@shinnn
Copy link
Contributor Author

shinnn commented Sep 10, 2014

@netroy What do you think?

try {
callback(null, lookup(buffer, filepath));
Copy link
Member

Choose a reason for hiding this comment

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

since lookup is a sync function, only one of the callback methods would be ever called...
can you add a test that is broken before this change & fixed after it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll do it. Done. #25 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

ah, took my slow brain a while to understand... Thanks for doing this.

Essentially we are handling errors from the callback function itself, which kinda feels dirty (handling someone else's errors)..

would it make sense if I change this to be something like this

var dimensions, err;
try {
  dimensions = lookup(buffer, filepath);
} catch (e) {
  err = e;
}
callback(err, dimensions);

solves the same issue without handling external errors.

PS: sorry for not being responsive on this PR, been crazy busy lately :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

err has been already defined here.
We need not to define it again.

var dimensions;
try {
  dimensions = lookup(buffer, filepath);
} catch (e) {
  err = e;
}
callback(err, dimensions);

@coveralls
Copy link

Coverage Status

Coverage increased (+0.11%) when pulling db56559 on shinnn:master into 43fc9ad on netroy:master.

@shinnn
Copy link
Contributor Author

shinnn commented Sep 10, 2014

@netroy

  1. I added the test to check if the callback function is called only once before fixing index.js. As we can see, it failed.

    https://travis-ci.org/netroy/image-size/jobs/34908820#L97

    Error: done() called multiple times
    
  2. After fixing index.js, it passed the test.

@shinnn
Copy link
Contributor Author

shinnn commented Sep 11, 2014

Fixed.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) when pulling a600057 on shinnn:master into 43fc9ad on netroy:master.

@netroy
Copy link
Member

netroy commented Sep 11, 2014

thanks for doing this..

netroy added a commit that referenced this pull request Sep 11, 2014
Do not call the callback function twice when image size detection failed
@netroy netroy merged commit ace9976 into image-size:master Sep 11, 2014
@shinnn
Copy link
Contributor Author

shinnn commented Sep 11, 2014

You're welcome :)

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.

None yet

3 participants