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

fishy response.end handling in connect middleware #87

Closed
agalazis opened this Issue Jan 24, 2017 · 11 comments

Comments

Projects
None yet
5 participants
@agalazis

agalazis commented Jan 24, 2017

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

This comment has been minimized.

Member

theganyo commented Jan 24, 2017

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

This comment has been minimized.

agalazis commented Jan 24, 2017

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

This comment has been minimized.

Member

theganyo commented Jan 24, 2017

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

This comment has been minimized.

agalazis commented Jan 24, 2017

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:
i18next/i18next-express-middleware#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

This comment has been minimized.

Member

theganyo commented Jan 24, 2017

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

This comment has been minimized.

agalazis commented Jan 24, 2017

yes exactly

@agalazis

This comment has been minimized.

agalazis commented Feb 1, 2017

not sure how is see you have already fixed this and in the end it's not there:
737f5fc#diff-53c9a79e933c4da44b7ab7ad372897b9

@filtersweep

This comment has been minimized.

filtersweep commented Feb 2, 2017

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

@robotrobot

This comment has been minimized.

robotrobot commented Jun 20, 2017

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

@jwalton

This comment has been minimized.

Contributor

jwalton commented Mar 15, 2018

@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 added a commit to jwalton/swagger-node-runner that referenced this issue Mar 15, 2018

Issue apigee-127#87: Do not call 'next()' after writing message body,…
… as this will likely cause other express handlers to attempt to write to the response after we've already written a response.
@jwalton

This comment has been minimized.

Contributor

jwalton commented Mar 15, 2018

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();
        }
    });

theganyo added a commit that referenced this issue Mar 19, 2018

Merge pull request #125 from jwalton/master
Issue #87: Do not call 'next()' after writing message body.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment