1

I've read other questions regarding this error, but I can't see how any of the answers apply to my own case. I've checked the usual culprits - I don't do anything with the response object after calling send and as far as I can tell my callback isn't being called more than once. But no matter what I try I keep getting the error "Can't set headers after they are sent". Here is an example of my code:

var express = require('express');
var ws = require('nodejs-websocket');
var app = express();
var socket = ws.connect("ws://localhost:5000");

var port = 8000;

app.get('/', function(req, res) {
  var r = JSON.stringify({
    type: "page request",
    request: {
      url: req.query.url,
      app: req.query.app
    }
  });
  socket.on("text", function(str){
    console.log(str);
    var d = JSON.parse(str);
    res.send(d.response.body);
    return;
  });
  socket.sendText(r);
});

app.listen(port, function(){
  console.log("Node app is running on port", port);
});

The first request to '/' works like a charm (or so it seems). Every time, however, the second request results in "Can't set headers after they are sent". Maybe I'm not understanding how the callback for the websocket is closing around the res object? Is it constantly referring to the original res object? I would have thought that each time the route was requested the callback would be redefined with the new res object, but maybe I'm wrong?

There are other issues here. If, for example, no response is received from the websocket then the response will never be returned, which is something I plan on addressing. Also, the way the code stands now any second message coming through the websocket could lead to the error, but I'm currently testing on my local machine with one connected client only, and I'm printing all messages to the console and can see that one and only one message is coming through during the request/response lifecycle. Any help is greatly appreciated!

h2ounderthebridge
  • 75
  • 5
  • Did you search for this specific error? There are probably 100 similar questions with that specific error here and the root cause is always the same. From the looks of your code, you need to learn about ***asynchronous*** responses too. Your code doesn't execute top to bottom. The async callbacks get called much later. – jfriend00 Apr 18 '16 at 21:30
  • And, you don't want to be doing `socket.on()` inside an `app.get()` handler. That adds a new event handler every time the route is hit. There are lots of things wrong with this code. We can't really help re-architect it because you don't describe the desired behavior. – jfriend00 Apr 18 '16 at 21:30
  • Possible duplicate of [Node.js Error: Can't set headers after they are sent](http://stackoverflow.com/questions/7042340/node-js-error-cant-set-headers-after-they-are-sent) – alexmac Apr 18 '16 at 21:30
  • @jfriend00 - well then that answers the question - so calling socket.on("text"... defines a second handler? It doesn't overwrite the first? That would explain it, obviously. And I'm aware the code doesn't execute top to bottom, I wouldn't want it to. – h2ounderthebridge Apr 18 '16 at 21:37
  • You also have concurrency issues because if multiple requests arrive on your server to the `/` route, you have no way of sorting out which `socket.on('text')` response belongs to which incoming http request and they can easily get mixed up. The design needs some work. – jfriend00 Apr 18 '16 at 21:42

1 Answers1

1

There are multiple issues here.

The problem with the second request is that socket.on(...) adds a new listener every time you call it. So, each app.get(), you add yet another socket.on() listener so they will pile up and you will get more than one called for each incoming message, leading to more than one attempt to send the response, leading to the specific error message you see. There is pretty much never a case where you can to add a listener in a request handler because they will accumulate every time the request is processed.

In addition, you have a concurrency issue here. If you have multiple app.get('/', ...) requests in process at the same time, you have no way of sorting out which socket.io response goes with which web request so they will likely get mixed up. You probably need to tag each socket request so when a response comes back, you know which request it belongs to and you can send the response to the right request.

jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • Thank you, that's the problem. I was incorrectly assuming that it was rewriting the listener, instead of adding a new one. I just added a line to call socket.removeListener inside the callback, which ensures that there is only ever one event listener. Thanks! – h2ounderthebridge Apr 18 '16 at 21:45
  • 1
    @h2ounderthebridge - I don't know how you attempted to fix your code, but you also may have a concurrency issue which I have added to my answer. – jfriend00 Apr 18 '16 at 21:47
  • thanks for the input. What I did was assign the callback to a variable named c, then inside the callback I just call "socket.removeListener('text', c)" to remove it after the response has come back. That way a callback which responds to just that particular connection is generated each time. Also, this is a single user (me) app, there won't ever be concurrent requests. I know this would be a bad idea with multiple users hitting the server. – h2ounderthebridge Apr 18 '16 at 21:49