search

apigee-127 / swagger-node-runner

The heart of Swagger-Node
MIT License
102 stars 124 forks source link

Error and attempted 404 after sending swagger spec #80

Open chrisegner opened 7 years ago

chrisegner commented 7 years ago

If you use a swagger_raw pipe to send out the swagger spec when mediated by connect_middleware, the request processing continues after the response has been sent. This eventually leads to express's default 404 handler kicking in, which in turn produces an error when it attempts to set the status code header for a response that has already been sent.

Specifically, the commit ac6dad9fb5071bddd7a95c32cc38e9cc15c57228 needs to be reverted. It's not clear what problem it's attempting to solve, so I can't say what the impact is, but whatever it's trying to do, this manner of doing it breaks express's contact.

Stack trace:

Error: Can't set headers after they are sent.
  at ServerResponse.OutgoingMessage.setHeader (http.js:690:11)
  at ServerResponse.res.setHeader (/development-source/web/node_modules/express/node_modules/connect/lib/patch.js:63:22)
  at next (/development-source/web/node_modules/express/node_modules/connect/lib/proto.js:156:13)
  at /development-source/web/node_modules/express/lib/application.js:121:9
  at next (/development-source/web/node_modules/express/node_modules/connect/lib/proto.js:129:23)
  at /development-source/web/node_modules/express/lib/application.js:121:9
  at next (/development-source/web/node_modules/express/node_modules/connect/lib/proto.js:129:23)
  at [object Object].finishConnect (/development-source/web/node_modules/swagger-node-runner/lib/connect_middleware.js:94:13)
<snip...>
theganyo commented 7 years ago

Could you take a look, @osher ?

chrisegner commented 7 years ago

I should probably clarify that, from the client side, this may cause a connection reset. We detected this when our test for our api spec endpoint returned ECONNRESET. It's possible a smaller document might have gotten out before the server crashed (I'm not super familiar the the internal details of the express and http modules' buffering schemes). For reference, our spec was 36k as yaml, 51k as json (endpoint serves json).

osher commented 7 years ago

@chrisegner - if you say that with a shorter yaml it works while with a heavier yaml it doesnt - it's a score for the buffer theory. I know that my end-to-end tests pass - however, they do work with a rather short spec doc. I'll be in the office tomorrow (I'm in GMT+2), and will be able to run some tests. I'll keep you updated with my findings as soon as I have any

osher commented 7 years ago

@chrisegner - man, I need help to reproduce. I've constructed an e2e test with a huge doc (over 100K), and did not encounter the bug.

can you please help provide a reproduction scenario?

If it helps, you can see what I've done here: https://github.com/theganyo/swagger-node-runner/pull/82

which is basically, raise a connect server with a document I inflated with a #definition/veryVeryBigCompoundType as a type with 200 string fields with a lorem-ipsum description (specifically 'content-length': '125454' for yaml, and a similar magnitude for json)

I could retrieve the doc both as yaml and as json

osher commented 7 years ago

@chrisegner - can you help me reproduce? maybe it's framework-dependent? could there be a collision with a middleware you're using? I would like to learn about it and solve it

osher commented 7 years ago

@chrisegner - what framework are you using? could it have any relation to Accept-Encoding: gzip? I may have found something, I need a little help from you to determine if what I found is relevant to your case or on another area

agalazis commented 7 years ago

could also have to do with https://github.com/theganyo/swagger-node-runner/issues/87 since he is using connect middleware

agalazis commented 7 years ago

I think the intention of that commit should be to put a return statement in the if and remove else but the first didn't happen going to do a pull request for this. I hope it's getting released soon