Uploading corrupt images with gm transformWrite will never finish the upload

[ghost]

When I upload images that gm/imagemagick can't handle (corrupt images, .ico) the upload will never finish. This has some bad consequences:

1. With cfs-s3 the upload stream to AWS S3 is never closed until a "socket hang up" error occurs because AWS closed the connection. This will restart the app because the error is not caught.
2. After restart collectionFS tries to upload the image again and fails.

Step 1. and 2. will repeat until I manually delete the image. collectionFS should handle this somehow.

[aldeed]

What happens when gm can't handle it? Does it throw an error we can catch and do cleanup? Otherwise I'm not sure what we could do since we blindly trust the transformWrite to pipe the stream.

[ghost]

The error gm convert: Read Exception (/var/folders/m3/2psq7dpx19178nx6x4cn142c0000gp/T/gmiCmxH5). is written to stderr. err is null.

var s3eventPhotosThumbnails = new FS.Store.S3('eventPhotosThumbnails', _.extend({}, s3options, {
  folder: S3_THUMBNAIL_SIZE_FOLDER,
  transformWrite: function (file, readStream, writeStream) {
    gm(readStream, file.name())
      .stream(function (err, stdout, stderr) {
        if (err) {
          console.error('gm error', err);
        }
        stderr.pipe(process.stderr);
        stdout.pipe(writeStream);
      });
  }
}));

Found a open bug regarding this: aheckmann/gm#256

[ghost]

I will try https://www.npmjs.org/package/imagemagick and check if the error reporting is better.
How can I tell collectionFS that there was an error in transformWrite and that the upload should be aborted. I want to show the user an error message that the photo is corrupt and couldn't be uploaded.

[aldeed]

Once you get to the transformWrite phase, the insert and upload is already complete, so the best you can do is indicate that the storage phase failed. It looks like the correct way to do this with our current logic is to throw an error from your transformWrite, although I don't see where we're doing any cleanup of the streams so we probably need to fix that. Relevant code is here: https://github.com/CollectionFS/Meteor-cfs-storage-adapter/blob/master/transform.server.js#L68

I think there is logic that automatically deletes the file after a few attempts to store it. That may or may not be working correctly at the moment.

[ghost]

Where can i catch for the exception thrown in createWriteStream?

[aldeed]

Right now you can't. It's called by the fileworker, which is like a separate server worker that does the saving to stores after upload, but we don't have proper error handling in there. We still plan to rewrite the fileworker, at which point we can handle save errors better.

I think we want to ultimately funnel the error back up to an onError handler on the collection.

[aldeed]

Closing. Discussion can continue on #227.