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

after enter an Error in fileFilter,it doesn't work #192

Closed
zyf0330 opened this issue Aug 5, 2015 · 31 comments
Closed

after enter an Error in fileFilter,it doesn't work #192

zyf0330 opened this issue Aug 5, 2015 · 31 comments
Labels

Comments

@zyf0330
Copy link

zyf0330 commented Aug 5, 2015

function fileFilter (req, file, cb) {

  // The function should call `cb` with a boolean
  // to indicate if the file should be accepted

  // To reject this file pass `false`, like so:
  cb(null, false)

  // To accept the file pass `true`, like so:
  cb(null, true)

  // You can always pass an error if something goes wrong:
  cb(new Error('I don\'t have a clue!'))

}

This line within 'new Error' works not well.
Though

res.render('error', {
      message: err.message,
      error: err
    });

has been executed in app.js error handle,but the page shows it is uploading file.

@LinusU
Copy link
Member

LinusU commented Aug 5, 2015

Hmm, I think that this is the same bug as in #168. I really need to get this fixed, hopefully I can get at least a workaround commited when I get home tonight.

Thank you for your patience.

@zyf0330
Copy link
Author

zyf0330 commented Aug 5, 2015

@LinusU Thanks,I have seen #168 and I think it is the same question also.
If you need some extra info about it, I am willing to help.

@LinusU
Copy link
Member

LinusU commented Aug 5, 2015

Thank you 👍

I don't have time to try anything at the moment, but if you want to experiment a bit, this is the lines where I think we need to change something: lib/make-middleware.js L36-42

When we unpipe the req, the browser never gets to send all of it's data. I tried adding a req.resume() but for some reason that didn't help. I don't know if it's because busboy is doing something strange.

Maybe something like this could work:

 function done (err) {
   if (isDone) return
   isDone = true
-  req.unpipe(busboy)
   busboy.removeAllListeners()
-  next(err)
+  busboy.on('finish', function () {
+    next(err)
+  })
}

@zyf0330
Copy link
Author

zyf0330 commented Aug 5, 2015

@LinusU it doesn't work,and the line 'next(err)' in callback is not went into.

@LinusU
Copy link
Member

LinusU commented Aug 6, 2015

Should be fixed by dfa2095, please reopen if issue persists.

@LinusU LinusU closed this as completed Aug 6, 2015
@zyf0330
Copy link
Author

zyf0330 commented Aug 7, 2015

@LinusU My problem is not about file size limit,and that don't fit me.

when I throw a error at FileFilter, line 95 in the make-middleware.js is ran into.

javascript
if (err) {
appender.removePlaceholder(placeholder)
return abortWithError(err)
}


then I don't know it do what!

@LinusU
Copy link
Member

LinusU commented Aug 7, 2015

Oh shit, I thought that it would be solved by that same bug. I'll take another look at this.

@LinusU LinusU reopened this Aug 7, 2015
@LinusU
Copy link
Member

LinusU commented Aug 7, 2015

@zyf0330 Could you try the following in make-middleware.js L95-98

         if (err) {
           appender.removePlaceholder(placeholder)
+          fileStream.resume()
           return abortWithError(err)
         }

@zyf0330
Copy link
Author

zyf0330 commented Aug 7, 2015

@LinusU it doesn't work too.What happened is that chrome shows the file is uploading after the error has been handled,and the progress keeps 14% all the time. So uploading stream needs to be closed,is it?

@LinusU
Copy link
Member

LinusU commented Aug 7, 2015

Hmm, could you try adding this in addition to the change that you already made.

make-middleware.js L31-37

     function done (err) {
       if (isDone) return
       isDone = true
       req.unpipe(busboy)
+      req.resume()
       busboy.removeAllListeners()
       next(err)
     }

@zyf0330
Copy link
Author

zyf0330 commented Aug 7, 2015

it can't do.uploading is continuing

@zyf0330
Copy link
Author

zyf0330 commented Aug 7, 2015

I write req.on('end') before req.resume(),but it can not run into.
As for busboy,I don't know it ,so I can't give some advice.

@LinusU
Copy link
Member

LinusU commented Aug 7, 2015

Okay, I'm pretty sure that this is a bug in Node.js itself 😭

See nodejs/node-v0.x-archive#25823

@zyf0330
Copy link
Author

zyf0330 commented Aug 12, 2015

I will put my eye on this issue.

@LinusU
Copy link
Member

LinusU commented Aug 12, 2015

Possible fix here: nodejs/node#2323. Haven't had the time to test yet thought...

@zyf0330
Copy link
Author

zyf0330 commented Aug 12, 2015

Let me have a look.wait.

@zyf0330
Copy link
Author

zyf0330 commented Aug 12, 2015

I change _stream_readable.js like the fix #2323 above,but nothing runs into lines which are changed and result is the same.
Maybe I do something wrong,after all I don't know many.

@LinusU
Copy link
Member

LinusU commented Aug 12, 2015

You still need this change to multer (I think), might be something more thought...

make-middleware.js L31-37

     function done (err) {
       if (isDone) return
       isDone = true
       req.unpipe(busboy)
+      req.resume()
       busboy.removeAllListeners()
       next(err)
     }

@zyf0330
Copy link
Author

zyf0330 commented Aug 12, 2015

Could you tell me if I should alter the multer\node_modules\busboy\node_modules\readable-stream\lib_stream_readable.js?

@LinusU
Copy link
Member

LinusU commented Aug 12, 2015

Hmm, I don't think so. It should be the stream from the builtin http module that exhibits the problem.

@zyf0330
Copy link
Author

zyf0330 commented Aug 12, 2015

Do you mean I need to alter that file and compile nodejs.exe?
Is it true?

@LinusU
Copy link
Member

LinusU commented Aug 12, 2015

I'm not sure on the specifics but, yeah, probably. That's why it's a bit of a pain to test it...

@zyf0330
Copy link
Author

zyf0330 commented Aug 12, 2015

That is a challenge for me...I think I need to think about it .

@LinusU
Copy link
Member

LinusU commented Aug 14, 2015

@zyf0330 Could you try #205 and see if it fixes it for you?

@zyf0330
Copy link
Author

zyf0330 commented Aug 15, 2015

I will at Monday.

@LinusU
Copy link
Member

LinusU commented Sep 19, 2015

This should be fixed by #205 and released on npm as version 1.0.5 🚀

Please reopen if issue persists.

@LinusU LinusU closed this as completed Sep 19, 2015
@tibortru
Copy link

tibortru commented Oct 7, 2015

I still have this issue. I am using multer 1.0.6 and node 0.12.7

i've got

fileFilter: function (req, file, cb) {

        if (file.mimetype !== 'image/png'
            && file.mimetype !== 'image/jpg'
            && file.mimetype !== 'image/jpeg'
            && file.mimetype !== 'image/gif') {
            console.log('Got file of type', file.mimetype);
            return cb(new Error('Only image files are allowed!'));
        }

        // To accept the file pass `true`, like so:
        cb(null, true);

    }

the function executes, but request goes Pending in Chrome,

@zyf0330
Copy link
Author

zyf0330 commented Oct 8, 2015

@tibortru May you can try node v4.0 version.

@tibortru
Copy link

tibortru commented Oct 9, 2015

@zyf0330 Ye, same thing... I actually don't know how I got this ancient version of Node installed... but after I reinstalled node to version 4.1.2 I still have same issue... Do I explicitly need 4.0 version of node?

EDIT: I have tried it on another PC where I got node 4.1.1 , still same issue

@chanlito
Copy link

chanlito commented Sep 9, 2016

guys is there a way to get res object inside fileFilter function arguments?

@LinusU
Copy link
Member

LinusU commented Sep 9, 2016

Why would you need that? It's purposely not included since you shouldn't respond to the request from the fileFilter function... Interested to hear your use case

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

No branches or pull requests

4 participants