bithavoc / express-winston

express.js middleware for winstonjs
https://www.npmjs.com/package/express-winston
MIT License
798 stars 186 forks source link

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

Closed mattieb closed 8 years ago

mattieb commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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.