Skip to content

Is formidable compatible with node 0.6? #130

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

Closed
manast opened this issue Feb 1, 2012 · 10 comments
Closed

Is formidable compatible with node 0.6? #130

manast opened this issue Feb 1, 2012 · 10 comments

Comments

@manast
Copy link

manast commented Feb 1, 2012

Hello,

I had a working code using formidable and express 2.5.1 with node 0.4.x. But now after updating to node 0.6.9, I sometimes get an error and other times I just dont get any parsed data at all, much in the same style as when bodyParser started to decode multipart. Of course I am not using bodyParser and I think the code looks correct, pretty much as the working examples out there. I am using formiddable 1.0.8.

Any ideas? the error looks like this:

[ERROR] console - Error: parser error, 0 of 16384 bytes parsed
at IncomingForm.write (/Users/manuel/dev/clouddisplay/node_modules/formidable/lib/incoming_form.js:141:17)
at IncomingMessage. (/Users/manuel/dev/clouddisplay/node_modules/formidable/lib/incoming_form.js:91:12)
at IncomingMessage.emit (events.js:67:17)
at HTTPParser.onBody (http.js:115:23)
at Socket.ondata (http.js:1403:22)
at TCP.onread (net.js:354:27)

@manast
Copy link
Author

manast commented Feb 1, 2012

ok, just as a complement note. I tried now with express.bodyParser and I get exactly the same error... so can we assume that this is an incompatibility with node 0.6?...

app.post('/upload/:uploadId', requiresLogin, express.bodyParser({ uploadDir: '/tmp' }),
function(req,res){
  console.log(req.files);
});

Any workarounds?

@mscdex
Copy link

mscdex commented Feb 2, 2012

What does requiresLogin do? Does it perform some async calls, such as to a database? If so, try temporarily removing requiresLogin from the app.post() call and see how that affects things.

@manast
Copy link
Author

manast commented Feb 2, 2012

Totally correct thank you very much. requiresLogin performs an asych database call, which somebody added without my knowledge :).

So it works now, but whats the explanation for this behavior?

On Feb 2, 2012, at 11:02 AM, Brian White wrote:

What does requiresLogin do? Does it perform some async calls, such as to a database? If so, try temporarily removing requiresLogin from the app.post() call and see how that affects things.


Reply to this email directly or view it on GitHub:
#130 (comment)

@mscdex
Copy link

mscdex commented Feb 2, 2012

What's happening is the http request's 'data' events are being emitted while the async call to the database is happening. By the time the database action completes, some or all of the form data is now gone and formidable has little to nothing to parse. Thus you have to have formidable start parsing right away.

@manast
Copy link
Author

manast commented Feb 2, 2012

I see. This sounds a bit scary, what happens if I have several hundred users concurrently?
Maybe while some users are uploading content others are doing operations that perform async operations, couldn't this behavior potentially happen in those situations as well?

On Feb 2, 2012, at 1:22 PM, Brian White wrote:

What's happening is the http request's 'data' events are being emitted while the async call to the database is happening. By the time the database action completes, some or all of the form data is then already "gone" and formidable has little to nothing to parse. Thus you have to have formidable start parsing right away.


Reply to this email directly or view it on GitHub:
#130 (comment)

@mscdex
Copy link

mscdex commented Feb 2, 2012

I'm not sure what you mean here. The missing 'data' events problem here is specific to each incoming http request. The issue has to do with the fact that there is no handler for the request's 'data' events when you go to do your async db calls.

If you don't want the upload to continue if they are not logged in, you can try doing incomingForm._parser.end(); to stop parsing (only if the entire form hasn't already been parsed). This will however emit an error if it is not finished parsing, so you'll probably need some flag to know when to ignore the error and when not to. You could probably also use this same technique if want to limit file sizes (I don't see a maxFileSize akin to maxFieldsSize for IncomingForm).

@manast
Copy link
Author

manast commented Feb 2, 2012

Yes, I was confused because I did not understand if this was a per request problem or, being node single threaded, other requests would also affect it.

Thanks for your efforts trying to enlighten me. I still don't grasp the reason for the failure, I would need to go through the source code to understand the details. Nevertheless It seems as a quite inconvenient limitation, if I understand it correctly the implication is that I can not use any asynchronous middleware with formidable. The obvious question that follows is if this problem is an architectural problem with express or formidable and if any solution is in the works?

On Feb 2, 2012, at 5:30 PM, Brian White wrote:

I'm not sure what you mean here. The missing 'data' events problem here is specific to each incoming http request. The issue has to do with the fact that there is no handler for the request's 'data' events when you go to do your async db calls.

If you don't want the upload to continue if they are not logged in, you can try doing incomingForm._parser.end(); to stop parsing (only if the entire form hasn't already been parsed). This will however emit an error if it is not finished parsing, so you'll probably need some flag to know when to ignore the error and when not to. You could probably also use this same technique if want to limit file sizes (I don't see a maxFileSize akin to maxFieldsSize for IncomingForm).


Reply to this email directly or view it on GitHub:
#130 (comment)

@mscdex
Copy link

mscdex commented Feb 2, 2012

This is not a problem specific to any module really. It's how node core works, since everything is asynchronous. TCP (and thus HTTP) streams in node emit 'data' events every time a chunk of data is received from the client (this can often happen within the same tick where the incoming connection event occurs). If nobody is listening for data on that stream, then the data is discarded. This technique allows for low memory usage since chunks are emitted as they come in and they are not stored or buffered up anywhere.

Express also does not buffer requests' 'data' events for the same reason node core doesn't (memory usage). Express could support something like that, but I doubt you'll see that added anytime soon.

Something else you might try is "pausing" the incoming request via req.pause();. This will cause no more 'data' events to be emitted from that request and will start telling the other side to stop sending data. However, one or more 'data' events may already be "in the pipeline" and will be emitted even after you pause the incoming request. After you have completed your authentication, you should then be able to do req.resume(); to resume the stream and continue emitted 'data' events.

So, you CAN use async middleware with formidable, but you need to have formidable load before any of these async middleware so that it can capture all of these 'data' events.

@mtoymil
Copy link

mtoymil commented Sep 28, 2012

I am a bit confused about how an async db operation prevents 'data' listeners from being set in time. They are async, right? Also, shouldn't this problem occur in all cases where you are setting 'data' listeners after making async db calls, not just with formidable?

@svnlto
Copy link
Contributor

svnlto commented Dec 7, 2012

might still work, but generally, no. 77a30ff

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

No branches or pull requests

4 participants