Skip to content
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

Can't set headers after they are sent. #255

Open
Natashkinsasha opened this issue Jan 13, 2019 · 8 comments
Open

Can't set headers after they are sent. #255

Natashkinsasha opened this issue Jan 13, 2019 · 8 comments

Comments

@Natashkinsasha
Copy link

Hi. I decided to try an example. But on the request I get 204 code and an error in the logs. How I understand it before my error handler, some other one works.

@controller('/foo')
class FooController implements interfaces.Controller {
    @httpGet('/')
    private index(req: express.Request, res: express.Response, next: express.NextFunction) {
        return next(new Error());
    }
}
const container = new Container();
const server = new InversifyExpressServer(container);
server.setErrorConfig((app) => {
    app.use((err: Error, req: express.Request, res: express.Response, next: express.NextFunction) => {
        res.status(500).send('Something broke!');
    });
});
const app = server.build();
app.listen(3000);
Error: Can't set headers after they are sent.
    at validateHeader (_http_outgoing.js:491:11)
    at ServerResponse.setHeader (_http_outgoing.js:498:3)
    at ServerResponse.header (E:\workplace\wx-meteor-v2\node_modules\inversify-express-utils\node_modules\express\lib\response.js:767:10)
    at ServerResponse.contentType (E:\workplace\wx-meteor-v2\node_modules\inversify-express-utils\node_modules\express\lib\response.js:595:15)
    at ServerResponse.send (E:\workplace\wx-meteor-v2\node_modules\inversify-express-utils\node_modules\express\lib\response.js:145:14)
    at app.use (E:\workplace\wx-meteor-v2\src\index.ts:37:25)
    at Layer.handle_error (E:\workplace\wx-meteor-v2\node_modules\inversify-express-utils\node_modules\express\lib\router\layer.js:71:5)
    at trim_prefix (E:\workplace\wx-meteor-v2\node_modules\inversify-express-utils\node_modules\express\lib\router\index.js:315:13)
    at E:\workplace\wx-meteor-v2\node_modules\inversify-express-utils\node_modules\express\lib\router\index.js:284:7
    at Function.process_params (E:\workplace\wx-meteor-v2\node_modules\inversify-express-utils\node_modules\express\lib\router\index.js:335:12)
@nref-dan
Copy link

nref-dan commented Jan 25, 2019

We are also having this issue. It appears to only affect the last listed method in the controller class.

I have found 2 workarounds but these go against the documentation of calling return next(err);.

Workaround 1 (throw the error, seems a bit pointless try/catch to throw, but you may want to manipulate the error before you send it up):

try {
  const val = await serviceCallAsync();
  res.status(200).send(val);
} catch(err) {
  throw err;
}

Workaround 2 (utilise the Promises):

try {
  const val = await serviceCallAsync();
  Promise.resolve(val);
} catch(err) {
  return Promise.reject(err);
}

We haven't been able to figure out where the problem lies but it could be something to do with the decorators.

@Jameskmonger
Copy link
Member

@PodaruDragos I have very recently been added as maintainer and I am going to start working through the backlog

@sittingbool
Copy link

Will there be an actual solution? I found that throwing an error works fine but not calling next

@SuspiciousLookingOwl
Copy link

Any update on this? The error handler doesn't seems to be working for me too.

@ctalvarez
Copy link

I'm having the same issue. I have this configuration to handle some method error with setErrorConfig and sometimes the server responds with status 204 No content, instead of the actual json and status. It always happens with the same endpoint, and with most of the others endpoints works fine, returning the corresponding error status

inversifyServer.setErrorConfig((app) => {
  app.use((err, req, res, _next) => {
    }
    // const jsonResponse ={ ... }
    return res.status(err.status || 500).json(jsonResponse);
  });


@httpPost(
    '/foo',
    'SomeMiddleware',
  )
  async Foo(req: Request, res: Response, next: NextFunction) {
    try {
      // do something
      return res.status(201).json();
    } catch (err) {
      return next(err);
    }
  }

The error in the console is Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
It looks like the response is sent before accessing the setErrorConfig callback.

@inversify inversify deleted a comment from kishorechavali Mar 3, 2021
@Mannydheer
Copy link

I'm having a super weird issue as well with this...

To give some context,

This is my implementation of overriding the default error handler:

export const errorHandler = (err: Error, req: Request, res: Response, next: NextFunction) => {
  //We check if the error is coming from our custom error objects.

  if (err instanceof HttpException) {
    return res.status(err?.statusCode).json({
      message: err?.message,
      errorName: err?.name,
      stack: err?.stack,
    });
  }
  //Errors that a unexpected and not handled by our custom errors.
  return res.status(500).json({
    message: err?.message,
    errorName: err?.name,
    stack: err?.stack,
  });

};

I have the following endpoint, let's call it endpoint1

 @httpPost('/refreshAccessToken')
  public async refreshAccessToken(req: Request, res: Response, next: NextFunction): Promise<any> {
    try {
      if (!req.body.refreshToken || !req.body.accessToken || !req.body.id || !req.body.email) {
        throw new BadRequestError('Missing fields')
      }
      return res.status(200).json({status: 200,message: 'Success getting new access token with refresh token.',});
    } catch (error) {
      next(error);
    }
  }

and then just below I have this empty endpoint, let's call this endpoint2.

  @httpPost('')
  public async() {
  }

If I have both endpoint1 and endpoint2, everything works fine, endpoint1 will throw a BadRequestError which will get caught in the catch block and then will be delegated to the errorHandler middleware.

As soon as I remove endpoint2, I get a 204 no-content and the following message:

ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client

I have no idea what is happening...

@arudenkoofficial
Copy link

I confirm the reports from @nref-dan and @Mannydheer
The error occurs for the last method listed in the controller.

Would be great to see some updates on this problem

@rsaz
Copy link
Contributor

rsaz commented Jul 16, 2024

The Real Issue & Solution

This issue is quite straightforward to resolve but comes with a philosophical twist. I tackled this in the ExpressoTS Framework, where you can seamlessly use hbs, ejs, express-handlebars, etc. While I could submit a PR, I’ve chosen not to—this is more of a philosophical battle I’d prefer to avoid.

The Issue

In Express.js, if you don’t explicitly end the response (by calling methods like res.send, res.json, res.end, etc.), the request will hang. This leaves it up to the developers to handle these cases. For example:

app.get("/", (req, res) => {
    return;
});

In inversify-express-utils the author of the lib decided to provide a solution for these use cases, which honestly, is a great idea. The author call in their code and tests "Falsey" return;

The idea behind automatically handling undefined return values with a 204 No Content status in inversify-express-utils is to prevent this hanging by default. There are few issues with his approach in terms of logic, and the way that the metadata is being handle to distinguish what is res.send, res.render and any other method with their unique needs, thus its indeed interfering with the way res.render works.

The file and method responsible for generating this situation is server.ts and the method is handlerFactory

Here is the code:

private handlerFactory(
    controllerName: string,
    key: string,
    parameterMetadata: ParameterMetadata[],
  ): RequestHandler {
    return async (
      req: Request,
      res: Response,
      next: NextFunction
    ): Promise<void> => {
      try {
        const args = this.extractParameters(req, res, next, parameterMetadata);
        const httpContext = this._getHttpContext(req);
        httpContext.container.bind<HttpContext>(TYPE.HttpContext)
          .toConstantValue(httpContext);

        // invoke controller's action
        const value = await (httpContext.container.getNamed<Controller>(
          TYPE.Controller,
          controllerName,
        )[key] as ControllerHandler)(...args);

        if (value instanceof HttpResponseMessage) {
          await this.handleHttpResponseMessage(value, res);
        } else if (instanceOfIHttpActionResult(value)) {
          const httpResponseMessage = await value.executeAsync();
          await this.handleHttpResponseMessage(httpResponseMessage, res);
        } else if (value instanceof Function) {
          value();
        } else if (!res.headersSent) {
          if (value === undefined) {
            res.status(204);
          }
          res.send(value);
        }
      } catch (err) {
        next(err);
      }
    };
  }

When you use res.render with or without return the value is undefined, always falling in to setting the response to 204 blocking the possibility to render the page right after set the header.

if (value === undefined) {
  res.status(204);
}
 res.send(value);

SOLUTION

Option 1 - Remove the "Falsey" validation and make sure that the inversify-express-utils is aligned with expressjs default behavior

private handlerFactory(
    controllerName: string,
    key: string,
    parameterMetadata: ParameterMetadata[],
  ): RequestHandler {
    return async (
      req: Request,
      res: Response,
      next: NextFunction
    ): Promise<void> => {
      try {
        const args = this.extractParameters(req, res, next, parameterMetadata);
        const httpContext = this._getHttpContext(req);
        httpContext.container.bind<HttpContext>(TYPE.HttpContext)
          .toConstantValue(httpContext);

        // invoke controller's action
        const value = await (httpContext.container.getNamed<Controller>(
          TYPE.Controller,
          controllerName,
        )[key] as ControllerHandler)(...args);

        if (value instanceof HttpResponseMessage) {
          await this.handleHttpResponseMessage(value, res);
        } else if (instanceOfIHttpActionResult(value)) {
          const httpResponseMessage = await value.executeAsync();
          await this.handleHttpResponseMessage(httpResponseMessage, res);
        } else if (value instanceof Function) {
          value();
        } else if (!res.headersSent) {
          if (value !== undefined) {
            res.send(value);
          }
        }
      } catch (err) {
        next(err);
      }
    };
  }

Secret sauce is right here in case you haven't see it

  } else if (!res.headersSent) {
    if (value !== undefined) {
      res.send(value);
    }

Option 2 - Make sure value can have a metadata or any data that can distinguish the method utilized in order to treat different those methods and their unique usecases.

Final considerations

If you decide to clone and build the lib by yourself, with the fix I use, pay attention to one particular test case in the serve.test.ts which validates "Falsey" return. This test obviously will fail

 it('should work for controller methods who\'s return value is falsey', done => {
      @controller('/user')
      class TestController {
        @httpDelete('/') 
        public async delete(): Promise<void> {
          return new Promise((resolve) => setTimeout(resolve, 100));
        }
      }

      server = new InversifyExpressServer(container);
      void supertest(server.build())
        .delete('/user')
        .expect(204, '', done);
    });
  });

I hope this sheds some light on the final solution for those trying to use render engines with inversify and express. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants