-
Notifications
You must be signed in to change notification settings - Fork 211
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
node 6 IncomingMessage events fix #134
Conversation
Can you provide a full example (including client side and a file) to reproduce the issue you're describing first? |
Ah so I was testing the above with curl, should have included $ curl -v -F userfile1=@/bin/echo http://localhost:8080 I'm unable to share the actual code I'm working on (it's for a book), but the side effect of this is it prevents the |
Your code example + that curl usage works for me. 'req end' is printed to the console. I tested with node v6.9.1. Out of curiosity, did you try moving your |
ok sorry - I changed the file to $ dd if=/dev/random bs=1 count=16383 > fail
$ dd if=/dev/random bs=1 count=16384 > pass
$ curl -v -F userfile1=@fail http://localhost:8080 # node v6 will not log req end, but v4 will
$ curl -v -F userfile1=@pass http://localhost:8080 # node v6 will log req end (as will v4) If you put res.end in the finish handler, the behavior is the same Given that it's actually related to watermark size, perhaps there is a better solution than |
@davidmarkclements the problem is that you are calling |
@mcollina - no that isn't it - this happens regardless of when res.end is called (you could wrap it in a 10 second setTimeout and it would make no difference) I've not explained the problem very well, just to outline it: The
This has unwanted side effects at higher levels, for instance the pattern of using |
I can duplicate it now. Whatever it is it seems to have started in node v4.5.0, node versions prior to that work fine. It appears that node is automatically
|
Ok, bisecting reveals it's this PR: nodejs/node#5419 EDIT: node issue posted here: nodejs/node#10344 |
Any update on this bug? I see that the node PR has been closed without a fix. |
There should no longer be a problem with this in |
Something seems to have changed between v4 and v6 around event ordering and piped streams,
The following will behave differently between v4 and v6 (the end event is ignored in v6)
Wrapping
boy.emit('finish')
in asetImmediate
solves this,I haven't found any side effects to doing this, although that doesn't mean there may be some