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

end doesnt fire if there are no files, but just fields. #16

Closed
kevbook opened this Issue Nov 20, 2013 · 16 comments

Comments

Projects
None yet
5 participants
@kevbook

kevbook commented Nov 20, 2013

No description provided.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Nov 20, 2013

Owner

Do you have particular form upload data you can post somewhere that reproduces the issue? There's already a test for fields only...

Owner

mscdex commented Nov 20, 2013

Do you have particular form upload data you can post somewhere that reproduces the issue? There's already a test for fields only...

@simonhambly

This comment has been minimized.

Show comment
Hide comment
@simonhambly

simonhambly Nov 21, 2013

I get the same happening. Using the html form in the GET in the 1st example in the busboy readme.md, enter a value into the textfield, do not select a file and submit the form. The upload data generated is:

------WebKitFormBoundary9z82Vwx112J5IrBW
Content-Disposition: form-data; name="textfield"

anything
------WebKitFormBoundary9z82Vwx112J5IrBW
Content-Disposition: form-data; name="filefield"; filename=""
Content-Type: application/octet-stream


------WebKitFormBoundary9z82Vwx112J5IrBW--

From what I can make out it looks like filefield causes a file of zero length to be opened and passed as a parameter in the 'file' event. In my middleware I ignore file with zero length so then end doesn't file.

Hope that helps — let me know if you need more.

simonhambly commented Nov 21, 2013

I get the same happening. Using the html form in the GET in the 1st example in the busboy readme.md, enter a value into the textfield, do not select a file and submit the form. The upload data generated is:

------WebKitFormBoundary9z82Vwx112J5IrBW
Content-Disposition: form-data; name="textfield"

anything
------WebKitFormBoundary9z82Vwx112J5IrBW
Content-Disposition: form-data; name="filefield"; filename=""
Content-Type: application/octet-stream


------WebKitFormBoundary9z82Vwx112J5IrBW--

From what I can make out it looks like filefield causes a file of zero length to be opened and passed as a parameter in the 'file' event. In my middleware I ignore file with zero length so then end doesn't file.

Hope that helps — let me know if you need more.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Nov 21, 2013

Owner

@simonhambly What if instead of ignoring it, you do file.resume(); to drain the stream? Do you receive 'end' then?

Owner

mscdex commented Nov 21, 2013

@simonhambly What if instead of ignoring it, you do file.resume(); to drain the stream? Do you receive 'end' then?

@simonhambly

This comment has been minimized.

Show comment
Hide comment
@simonhambly

simonhambly Nov 21, 2013

Just trying...

simonhambly commented Nov 21, 2013

Just trying...

@simonhambly

This comment has been minimized.

Show comment
Hide comment
@simonhambly

simonhambly Nov 21, 2013

@mscdex Perfect! calling file.resume() does trigger the 'end' event. Thanks.

simonhambly commented Nov 21, 2013

@mscdex Perfect! calling file.resume() does trigger the 'end' event. Thanks.

@kevbook

This comment has been minimized.

Show comment
Hide comment
@kevbook

kevbook Nov 21, 2013

@mscdex @simonhambly that approach doesn't sound right, I don't want to manually trigger 'end' event because what if there were fields that still need to be parsed.

kevbook commented Nov 21, 2013

@mscdex @simonhambly that approach doesn't sound right, I don't want to manually trigger 'end' event because what if there were fields that still need to be parsed.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Nov 21, 2013

Owner

@kevbook This "issue" was discussed awhile back when people complained about the 'end' event triggering before the streams were finished. Emitting 'end' once all the streams are finished can simplify things for busboy end users as it means storing less state (as long as you're using the streams in some way when they come in).

How about reverting the semantics of 'end' back to meaning "end of parsing" and then have a 'finish' or similar event for when all emitted file streams have ended? Or vice versa?

Owner

mscdex commented Nov 21, 2013

@kevbook This "issue" was discussed awhile back when people complained about the 'end' event triggering before the streams were finished. Emitting 'end' once all the streams are finished can simplify things for busboy end users as it means storing less state (as long as you're using the streams in some way when they come in).

How about reverting the semantics of 'end' back to meaning "end of parsing" and then have a 'finish' or similar event for when all emitted file streams have ended? Or vice versa?

@simonhambly

This comment has been minimized.

Show comment
Hide comment
@simonhambly

simonhambly Nov 21, 2013

No, I think it should stay as it is now with 'end' firing when all the files have been streamed.

That way you can ensure you've 'saved' the files before calling the next middleware. For example, I'm streaming the files into mongodb (with gridfs-stream) and I need to wait until the writestream closes and it returns the metadata about the stored files before I call the next middleware. Firing 'end' at end of parsing would make this v. difficult/impossible.

@kevbook I think it only appears to manually trigger the 'end' event because the there's 1 stream and it is drained. If there were other streams that had yet to be drained the 'end' would wait until all of streams (and the form) had been processed.

@mscdex However having to explicitly call file.resume() on zero length stream seems unnecessary. Wouldn't it be cleaner if the 'file' event was not fired if the stream had zero length?

simonhambly commented Nov 21, 2013

No, I think it should stay as it is now with 'end' firing when all the files have been streamed.

That way you can ensure you've 'saved' the files before calling the next middleware. For example, I'm streaming the files into mongodb (with gridfs-stream) and I need to wait until the writestream closes and it returns the metadata about the stored files before I call the next middleware. Firing 'end' at end of parsing would make this v. difficult/impossible.

@kevbook I think it only appears to manually trigger the 'end' event because the there's 1 stream and it is drained. If there were other streams that had yet to be drained the 'end' would wait until all of streams (and the form) had been processed.

@mscdex However having to explicitly call file.resume() on zero length stream seems unnecessary. Wouldn't it be cleaner if the 'file' event was not fired if the stream had zero length?

@kevbook

This comment has been minimized.

Show comment
Hide comment
@kevbook

kevbook commented Nov 21, 2013

@simonhambly I agree.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Nov 22, 2013

Owner

@simonhambly There's no way to know if a part is going to be zero length in advance. Besides even if there was something in place to prevent these kinds of parts, I think hiding zero-length file parts would be too much of a surprise to most/new users.

Owner

mscdex commented Nov 22, 2013

@simonhambly There's no way to know if a part is going to be zero length in advance. Besides even if there was something in place to prevent these kinds of parts, I think hiding zero-length file parts would be too much of a surprise to most/new users.

@simonhambly

This comment has been minimized.

Show comment
Hide comment
@simonhambly

simonhambly Nov 22, 2013

Makes sense — guess that's streams for you :)

simonhambly commented Nov 22, 2013

Makes sense — guess that's streams for you :)

@amir-arad

This comment has been minimized.

Show comment
Hide comment
@amir-arad

amir-arad Nov 23, 2013

Contributor

Hi.
Here's a fix that worked for me:

busboy.on('file', function(x, stream, filename) {
if (typeof filename === 'undefined') {
// submitted file field with no file in it.
// consume stream so busboy will finish
stream.on('readable', stream.read);
} else {
... the usual stuff....
}});

Contributor

amir-arad commented Nov 23, 2013

Hi.
Here's a fix that worked for me:

busboy.on('file', function(x, stream, filename) {
if (typeof filename === 'undefined') {
// submitted file field with no file in it.
// consume stream so busboy will finish
stream.on('readable', stream.read);
} else {
... the usual stuff....
}});

@amir-arad

This comment has been minimized.

Show comment
Hide comment
@amir-arad

amir-arad Nov 23, 2013

Contributor

looking at the common usage scenario I'd also suggest optionally silencing 'empty' file events.
This can be either opt-in or opt-out, but it makes sense that a file field with no filename is (assuming the client is a browser) an empty field.
In that case the stream should be drained and the event silenced.

At the very least, one can add this scenario to the readme..?

Contributor

amir-arad commented Nov 23, 2013

looking at the common usage scenario I'd also suggest optionally silencing 'empty' file events.
This can be either opt-in or opt-out, but it makes sense that a file field with no filename is (assuming the client is a browser) an empty field.
In that case the stream should be drained and the event silenced.

At the very least, one can add this scenario to the readme..?

@jonathanong

This comment has been minimized.

Show comment
Hide comment
@jonathanong

jonathanong Nov 30, 2013

i'd prefer if they were silenced, but this issue should be added to the read me either way. makes sense to not fix this if you want busboy to remain minimal - easy enough to avoid with docs.

jonathanong commented Nov 30, 2013

i'd prefer if they were silenced, but this issue should be added to the read me either way. makes sense to not fix this if you want busboy to remain minimal - easy enough to avoid with docs.

@amir-arad

This comment has been minimized.

Show comment
Hide comment
@amir-arad

amir-arad Dec 4, 2013

Contributor

On that matter, I don't find any workable solution for empty forms. submitting an empty form (no fields, no files) will not emit the 'done' event, forcing the server to wait until timeout.

Contributor

amir-arad commented Dec 4, 2013

On that matter, I don't find any workable solution for empty forms. submitting an empty form (no fields, no files) will not emit the 'done' event, forcing the server to wait until timeout.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Dec 4, 2013

Owner

@amir-arad The empty form issue should be fixed now in 218d60c.

Owner

mscdex commented Dec 4, 2013

@amir-arad The empty form issue should be fixed now in 218d60c.

@mscdex mscdex closed this Dec 13, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment