apigee-127 / swagger-node-runner

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

fishy response.end handling in connect middleware #87

Closed agalazis closed 6 years ago

agalazis commented 7 years ago

Hello I was working on an express project that uses custom middleware after swagger is added and realised that there exist issues with the provided connect middleware. I have just spotted the exact line in the source code: https://github.com/theganyo/swagger-node-runner/blob/master/lib/connect_middleware.js#L91 In the above line, response.end is called without returning and as a result next is called afterwards. I consider this as unexpected behaviour as the middleware is supposed either render response or call next and not both. The result is that request is unexpectedly processed by middleware loaded after swagger and in cases where that middleware responds to the user as well you get a 'Can't set headers after they are sent.' error.

theganyo commented 7 years ago

You're absolutely right, this is clearly a bug. Thanks for reporting it. It looks like the correct thing here would be to populate the response body without calling .end().

agalazis commented 7 years ago

In that case when would end be expected to to get called?(to me it seems that there is only a return statement missing after .end() is called )

theganyo commented 7 years ago

Well, this is the long-standing argument... Some people want the framework to end it, others want the framework to allow connect handlers after the fact. It seems to me that after the fact is more flexible.

As to when it would get called, I believe Express, at least, will automatically do that if a response was populated in the handler chain... but we'd need to be certain of it and the other frameworks.

agalazis commented 7 years ago

My initial question was why call next? and not why call end.

It doesn't make sense for any reason to run middleware after res.send/res.end/res.json for the only reason that the request life-cycle is done.

Practical Example : By convention a server can have a 'not found hanlder' middleware in case there is no middleware /matching path for the request to be served(a lot of scripts that bootstrap express apps generate that by default). If any middleware calls next after rendering it causes errors as you cannot re-respond with the not found message but that middleware gets called and tries to do so.

Another downside is the overhead of processing unneeded middleware. Response was served mission accomplished.

It's a common practice not to call next when you render for the above reasons I also had similar issues with other third party middleware that was patched: https://github.com/i18next/i18next-express-middleware/issues/19

As I understand you want to write a response and leave everything in the middle because you want to be flexible. In that case I would prefer no response and some clear result set in req or even better req.swagger. Up-to setting some headers and calling next sounds normal to me but I haven't encountered any middleware that leaves rendering in the middle and calls next or renders and calls next(feel free to give some examples if you have seen this)

theganyo commented 7 years ago

Yes, I think that's a fair argument. I was actually thinking of different cases that don't apply here anyway. Another argument for simply calling end() is that when you start writing to the body, you may actually be starting the process of responding to the client regardless... in which case you're back to the error you described above.

agalazis commented 7 years ago

yes exactly

agalazis commented 7 years ago

not sure how is see you have already fixed this and in the end it's not there: https://github.com/theganyo/swagger-node-runner/commit/737f5fcd1ed399e3b69a439c562d6eb7a37dc5ad#diff-53c9a79e933c4da44b7ab7ad372897b9

filtersweep commented 7 years ago

Is there an ETA for a release that contains this patch?

scaleupcto commented 7 years ago

Just wondering on an ETA too as we're seeing this error.

jwalton commented 6 years ago

@agalazis It looks like it was fixed in 0.7.1, but if you install 0.7.1 and have a look, it is definitely not fixed. Also, this bug is still here in master:

https://github.com/theganyo/swagger-node-runner/blob/master/lib/connect_middleware.js#L99-L101

jwalton commented 6 years ago

Super ugly hack while you're waiting for that PR to be merged. This needs node v0.9.3 or higher:

    swaggerExpress.register(app);

    app.use((req, res, next) => {
        if(res.headersSent) {
            // Hack to get around
            // https://github.com/theganyo/swagger-node-runner/issues/87.
            // If headers have already been sent, it means swagger-node-runner
            // has written a response and called `next()` incorrectly.  Just drop
            // the request.
        } else {
            next();
        }
    });