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

if res.end is called with an incomplete JSON body, server crashes #112

Closed
mattieb opened this issue Jun 1, 2016 · 2 comments
Closed

if res.end is called with an incomplete JSON body, server crashes #112

mattieb opened this issue Jun 1, 2016 · 2 comments

Comments

@mattieb
Copy link

mattieb commented Jun 1, 2016

See https://gist.github.com/zigg/06e891e55f22b2d0afebaaf6fc245408

The example may seem a bit contrived, but I've observed the behavior using express-winston with Loopback, possibly due to streaming behavior from that framework.

I'd be happy to make a PR to address this, but it might be simple enough to just fix.

@rosston
Copy link
Collaborator

rosston commented Jun 1, 2016

Thanks for the great test case! It blows up on the JSON.parse at https://github.com/bithavoc/express-winston/blob/v1.4.0/index.js#L243. It'll be simple to keep the code from blowing up (and add a test for it), so I should be able to have a fix pushed by tomorrow night.

However, one would also expect (I presume) in your test case that the logger would log {"foo":"bar"} as the response body. What it would do now—if it wasn't blowing up—is just log }. Fixing that to return the correct body will probably take a bit longer.

@rosston
Copy link
Collaborator

rosston commented Jun 2, 2016

Fix released in v1.4.1.

With this fix, your test case would log only '}' as the response body. I realize that logging the entire streamed body is probably desirable, but I'm a little worried about how to do that well. This is the main problem: if the developer expects that streaming a JSON response will require less memory than sending it all at once, we've violated that expectation by (re-)building up the entire response body as it gets streamed out.

My concerns aside, if logging the streamed response body is something you'd really like, feel free to open a separate issue for that.

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

2 participants