NodeJs + Express + Response after event callback

Question

In order to organize code (& me being a java developer), I've divided my code into Routes & Services.

Disclaimer: I'm a nobody on Node.

Service Code:

UserService.prototype.findUser = function(userId) {
  var self = this;
  container.db.users.findOne({
    userId : userId
  }, function(err, user) {
    self.emit('userFetched', err, user);
  });
};

Router code:

var login = function(req, res) {
  service.on('userFetched', function(err, user) {
    loginAndRedirect(err, user, req, res);
  });
  service.getUser(req.body.username, req.body.password);
};

When a login request comes, I get the error:

http.js:689
    throw new Error('Can\'t set headers after they are sent.');
          ^
Error: Can't set headers after they are sent.
    at ServerResponse.OutgoingMessage.setHeader (http.js:689:11)
    at ServerResponse.res.setHeader (C:\Users\O603088\Documents\Workspace\Learning\Shory\node_modules\express\node_modules\connect\lib\patch.js:59:22)
    at ServerResponse.res.set.res.header (C:\Users\O603088\Documents\Workspace\Learning\Shory\node_modules\express\lib\response.js:518:10)
    at ServerResponse.res.location (C:\Users\O603088\Documents\Workspace\Learning\Shory\node_modules\express\lib\response.js:652:8)
    at ServerResponse.res.redirect (C:\Users\O603088\Documents\Workspace\Learning\Shory\node_modules\express\lib\response.js:694:8)
    at loginAndRedirect (C:\Users\O603088\Documents\Workspace\Learning\Shory\server\routes\auth.js:24:9)
    at UserService.<anonymous> (C:\Users\O603088\Documents\Workspace\Learning\Shory\server\routes\auth.js:14:5)
    at UserService.EventEmitter.emit (events.js:117:20)
    at C:\Users\O603088\Documents\Workspace\Learning\Shory\server\services\UserService.js:19:10
    at callback (C:\Users\O603088\Documents\Workspace\Learning\Shory\node_modules\nedb\lib\executor.js:26:17)

One of the reasons I think is because login function ends before the callback executes, hence the response has already been generated. Hence, when the redirect is executed, the about error is thrown.

However, this works:

var login = function(req, res) {
  service.getUser(req.body.username, req.body.password).on('userFetched',
      function(err, user) {
        loginAndRedirect(err, user, req, res);
      });
};

UserService.prototype.getUser = function(username, password) {
  var self = this;
  container.db.users.findOne({
    username : username,
    password : password
  }, function(err, user){
    self.emit("userFetched", err, user);
  });
  return this; // Added this for method chaining
};

1. Why am i getting the error in the first technique?
2. Will the second approach always work? Or is it just a coincidence that this is working?

I know the fool-proof way of doing this is to pass a callback function to findUser method and call it from within findUser method, but i don't like that approach very much.

Answer 1

As your error shows :

 throw new Error('Can\'t set headers after they are sent.');

Means you are trying to send response two times one back the other.

May be in your code:

var login = function(req, res) {
  service.on('userFetched', function(err, user) {
    loginAndRedirect(err, user, req, res);-----------------------> 1st response sent
  });
  service.getUser(req.body.username, req.body.password);-------------> 2nd response sent.
};

Check for whether you are sending response simultaneously..

Answer 2

Will the second approach always work? Or is it just a coincidence that this is working?

I think it's a coincidence. The approach is the same. The reason why you are having problems is that you have a concurrency problem. If you only make one request at a time, this will work fine. If you have multiple requests which overlap, then you may run into this problem.

I think the issue is with this line:

service.on('userFetched', function(err, user) {
  loginAndRedirect(err, user, req, res);
});

The problem with this approach is that callback will fire when any userFetched event is emitted, not just the one corresponding to the request you have a closure around. When you call .on, you are giving the UserService a reference to your callback and it keeps the reference. In fact, it will keep it forever.

So, request 1 comes in, asks for a user, then listens for userFetched event, receives the event, and sends the data to response 1. So far so good.

Then request 2 comes in, asks for a user, then listens for userFetched event. The callback for request 1 sends the data to response 1, then the callback for response 2 sends the data to response 2. The first part will throw an error (the one you reported above). Depending on how the error handling is setup, the response 2 part may or may not actually occur.

Since every request adds an event listener and never removes it, eventually your heap will fill up with event listeners, all of which have closures around the request and response objects, preventing them from being garbage collected.

My guess is that if you run this program long enough, you will get the JavaScript equivalent of an OutOfMemoryError.

I know the fool-proof way of doing this is to pass a callback function to findUser method and call it from within findUser method, but i don't like that approach very much.

If you are struggling with a way that won't work and know a way that works and is the documented, known, good way to do it, then why are you fighting it? A one-time callback is exactly the way to solve this problem. You're making your life harder by fighting it.