Meteor-Community-Packages / Meteor-CollectionFS

Reactive file manager for Meteor
MIT License
1.05k stars 237 forks source link

[1.0.0] Add better error trace on transform streams #227

Open raix opened 10 years ago

raix commented 10 years ago

Issues like #225 are a bit hard to debug, there should be a better error message explaining that the Error toke place inside eg. transformWrite and hand the error message and trace if available.

ghost commented 10 years ago

I've created something for this that could be included in the package. Just want your feedback before I refactor it into the cfs-storage-adapter.

How can the transformWrite function notify that an error has occured?
What will the error handler do?
Open
My current transformErrorHandler code
/**
 * Wrapper that handles errors that are thrown while transforming.
 * It saves the error in the copy and emits stored.
 * @param {string} storageName The name of the storage.
 * @param {Function} transformFunction The transformation function.
 * @returns {Function}
 */
var handleTransformErrors = function (storageName, transformFunction) {
  return function (file, readStream, writeStream) {
    var handleError = function (error) {
      if (_.isObject(error) && error.errorType === 'Meteor.Error') {
        error = _.pick(error, 'error', 'reason', 'details');
      }
      console.log('handleTransformError', error);
      file.copies[storageName].error = error;
      writeStream.emit('error', error);
      readStream.unpipe(writeStream);
      writeStream.emit('stored', {
        fileKey: '',
        size: 0,
        storedAt: new Date()
      });
    };
    try {
      transformFunction(file, readStream, writeStream, handleError);
    } catch (error) {
      handleError(error);
    }
  };
};
Usage
var eventPhotos = new FS.Store.S3('eventPhotos', {
  // Options...
  transformWrite: handleTransformErrors('eventPhotos', function (file, readStream, writeStream, handleError) {
    // Transform code.
    // Trigger error with throw:
    throw new Meteor.Error(500, 'Transform failed');
    // Or trigger error with callback:
    handleError(new Meteor.Error(500, 'Transform failed');
  })
});
aldeed commented 10 years ago

@raix may have comments, but I think your ideas generally make sense. If you want to put together a PR, we might have more comments on the actual implementation of it. The important point is to properly catch all errors and emit "error" instead of "stored". You don't have to worry about funneling the error back to the collection because we can do that when we add the collection emitter feature. I agree that adding an error property to copies[store] is probably the best way to get the error back to the client. It might actually be best to save the entire error object there rather than just the message. I think EJSON handles that properly, but not sure.

piyushcoader commented 10 years ago

Error: Error storing file to the Uploads store: socket hang up error and i have not used transformWrite

Error: Error storing file to the Uploads store: socket hang up W20140824-11:14:45.598(5.75)? (STDERR) at null. (packages/cfs-collection/common.js:88) W20140824-11:14:45.598(5.75)? (STDERR) at emit (events.js:106:17) W20140824-11:14:45.599(5.75)? (STDERR) at Writable. (packages/cfs-storage-adapter/storageAdapter.server.js:203) W20140824-11:14:45.599(5.75)? (STDERR) at Writable.emit (events.js:117:20) W20140824-11:14:45.599(5.75)? (STDERR) at Response. (packages/cfs-s3/s3.upload.stream2.js:120) W20140824-11:14:45.599(5.75)? (STDERR) at Request. (/Users/piyushthapa/.meteorite/packages/cfs-s3/CollectionFS/Meteor-cfs-s3/97d3c544165c766115f2b91a6aaca54a7438a558/.build/npm/node_modules/aws-sdk/lib/request.js:263:18) W20140824-11:14:45.600(5.75)? (STDERR) at Request.callListeners (/Users/piyushthapa/.meteorite/packages/cfs-s3/CollectionFS/Meteor-cfs-s3/97d3c544165c766115f2b91a6aaca54a7438a558/.build/npm/node_modules/aws-sdk/lib/sequential_executor.js:117:20)

cristiandley commented 10 years ago

im having the same issue only under meteor-up

GET http://mydomain.com/sockjs/info?cb=7sbwqvwd9v net::ERR_CONNECTION_REFUSED 
Error: "Queue" network [undefined], Error
    at http://mydomain.com/d4113048f6873f06ed8b5ce655e8b1f5a39feb00.js:22:23751
    at http://mydomain.com/d4113048f6873f06ed8b5ce655e8b1f5a39feb00.js:22:21121
    at http://mydomain.com/d4113048f6873f06ed8b5ce655e8b1f5a39feb00.js:1:8368
    at XMLHttpRequest.v.onreadystatechange (http://mydomain.com/d4113048f6873f06ed8b5ce655e8b1f5a39feb00.js:22:22428) 

this is my issue issue on mup

RobertLowe commented 9 years ago

+1, this is really needed...

My natural thought would be that collection.insert(file, function(err)(){ }) would contain error(s) information.

I'd accept any error handling really...

PS: Has any work been started on this?

evolross commented 9 years ago

Hey Sanjo,

If I wanted to use your handleTransformErrors - how would I do that? Do you have a repository forked with it included - or can I edit it myself? Not sure where to put your function. And I'd really love to pass the error back to the user on a bad transform.

Thanks.

ghost commented 9 years ago

@evolross Just copy the code into your project. I had it in the file where I did all the CollectionFS stuff. The code is licensed under MIT license.

evolross commented 9 years ago

I got something working using your above handleTransformErrors function. Thank you! I did have to call handleError() to trigger the error versus just throw new Meteor.Error(). It wouldn't work right using the latter.

Since the error is ultimately set in copies.store.error one tricky thing I had to do on the client in order to check for the error was to create a Tracker.autorun after the file is inserted since in the callback of the File.insert in result, copies is not yet available. That only shows up on the client after it's uploaded and stored some moments later. So the autorun checks for Images.findOne(id).copies == null. Once that fails, I check for copies.store.error and stop the autorun. Not sure if this is the best way to do it, but it works.