fix: we have one failing test

[NoNameProvided]

One of our test is failing in the last few commit. We should investigate and fix that.

[MichalLytek]

I use node v8.1.4 and don't have any failing test. I think that the newest version of node have broken some internals API so we should try to update our dependencies to the newest versions.

[NoNameProvided]

Here is the latest build: https://travis-ci.org/pleerock/routing-controllers/jobs/261758848

Our @Session(param) should throw required error when param is empty tests fail with a Socket hang up error.

[MichalLytek]

Ok, I've updated local node version and on v8.2.1 I have the test failing. But I have no idea why the socket hang, tried deps update but no success 😞

[pleerock]

If this issue with @Session then @19majkel94 you have to figure out solution since its your creation 😆

[MichalLytek]

😆
I've only done case "session": return req.session. I don't find myself guilty as the test mystically fail with closing server, socket and connection 😨

[patrickhousley]

Just quickly looking at the issue, I found that commenting out line https://github.com/pleerock/routing-controllers/blob/master/src/driver/express/ExpressDriver.ts#L336 results in the test passing.

[NoNameProvided]

I don't find myself guilty as the test mystically fail with closing server, socket and connection 😨

No body is guilty of anything in OSS. 🙂 @pleerock simply said you should check it out because you wrote it, so probably you are the one who can find out where the issue commings from.

Just quickly looking at the issue, I found that commenting out line

Thanks for looking into this @patrickhousley do you want to follow up on your findings and create a PR to fix it?

[patrickhousley]

Honestly, the fix would be to not call next if routing-controllers handles the error. This is standard for all express error handlers. Simply put, you should never try to modify the response, send headers, or send more data after the first call to send. The fact that this works in Node 8.1 and not in 8.2 is a fluke. They updated 8.2 here to prevent this.

[patrickhousley]

This is the actual PR that was merged into Node.

[patrickhousley]

@19majkel94 How would you like to proceed on this one. It seems it is going to need a, possibly drastic and breaking, change to the code base.

[NoNameProvided]

Honestly, the fix would be to not call next if routing-controllers handles the error. This is standard for all express error handlers.

I cannot find it now, but this was discussed. It is a needed bad coming from some implementation detail of the library.

It seems it is going to need a, possibly drastic and breaking, change to the code base.

Just +1 reason to rewrite the middleware execution.

I think we should move forward with this after making our decision on #107, but that will need some more discussion. For now only @19majkel94 and me made our decision.

[patrickhousley]

After giving this a little more thought, I believe it is certainly possible that an error handler may send headers and still pass the error on (logging is an excellent example). We also cannot always assume that third party error handlers are not going to pass the error on to next. The root cause of this error is here (I believe).

[patrickhousley]

For now, anyone coming across this can use the below code to prevent the issue with the socket being closed. Make sure this is the last error handler added to the Express app.

/**
   * Add a final error handler that will prevent the Express final error handler from executing
   * when routing-controllers error handler or any other error handler has already sent headers
   * and data.
   * See https://github.com/pleerock/routing-controllers/issues/243
   */
  app.use((error: Error, _req: express.Request, res: express.Response, next: express.NextFunction) => {
    if (res.headersSent || res._header) {
      return next();
    }

    return next(error);
  });

Custom type definition to support access to res._header:

import * as express from 'express';

module 'express' {
  interface Response implements express.Response {
    _header: any;
  }
}

[NoNameProvided]

Nice found! For me this boils down to the same thing still, the same you mentioned. We should rethink how and where we call next and prevent calling it after the last middleware.

[MichalLytek]

the fact that this works in Node 8.1 and not in 8.2 is a fluke. They updated 8.2 here to prevent this.

Node v8.3.0 and still the same failing test problem 😢

[NoNameProvided]

Does anyone using sessions here? I just tested it seems the functionality is totally broken in express.

[MichalLytek]

I just tested it seems the functionality is totally broken in express.

So why the rest of session test cases doesn't fail? The only problem is that the serwer hang up when we want to load param from session (@Session("empty")) which is undefined and this action param is required. Do we have any other test case for throwing Error 400 on undefined required param? Maybe this functionality is broken, not the session stuffs.

[fabiob]

I've found a simple solution for this bug (see 4b3f2e5). However, it conflicts with some assumptions (that routing-controllers error handler should call express' global error handler).

Is this really needed?

[patrickhousley]

@fabiob The issue with the change you linked to is that custom error handlers will never be called. Honestly, I think the correct fix is to make the routing-controllers error handler the last handler to be called and then have it not pass the error on the the express error handler.

[MichalLytek]

@pleerock @NoNameProvided can we just comment the test case that fails? Until we found a fix for newest node issue we should disable it because it's easy to miss the not falsy-negative test like in #269 it took place.

[pleerock]

someone said its because of next call after routes, am I right or I misunderstood something? If yes, its the reason then we decided to remove next call, right?

[MichalLytek]

I can't confirm that, the problem may be that we send error.next and the express-session is trying to handle the error but headers already sent and on newset node this is unhandled exception which terminate the app and the connection.

We should comment it until we fix it, there are many paralel PRs and features/fixes, not calling next = big changes in middlewares as I described in other issue.

[pleerock]

okay comment it, we should not have "build: failed" even few hours, but we had it for months... But dont forget to return back to that test and fix related issues later 😆

[MichalLytek]

Looks so much better 😀
https://travis-ci.org/pleerock/routing-controllers/builds/272181331

[NoNameProvided]

I dont like that we commented in, I bet on it it wont be fixed again. This way we lost coverage for a feature so now we can broke it for everyone without noticing. We should really dig and fix that.

I would say I will give a try but I am so short on time nowadays because of coming releases that I barely have time to read through my notifications.

[pleerock]

Agree that we should not comment it, however having a build failed also not good and unfortunately looks like noone has time to fix it =(

[MichalLytek]

I bet that the problem is with default error handler which bubble the error so the session middleware is trying to react on the error but it is already handled.

[github-actions]

Stale issue message

[NoNameProvided]

Closing as out-of-date.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.