bobbyz bobbyz - 5 months ago 8
Node.js Question

Best way to avoid "Cannot set headers after they are sent" error

I've read a few of the responses to this question on S.O. (here) and here, and I'm wondering if this is the best way to do this, or if there is a more universally-accepted best practice for this. So if I have the following code, for example:

app.delete('/something/:somethingId', function (req, res) {
for (var i = 0; i < thingList.length; i++) {
if (thingList[i].id === req.params.somethingId) {
thingList.splice(i, 1);
res.send(thingList[i]);
}
}
res.send("Nothing with that ID found");
});


This errors with a "Cannot set headers after they are set" I believe because
res.send()
is asynchronous and this code will try to run the outer
res.send()
while the inner one has already begun the send process. However, I can't put the outer
res.send()
in an
else
block because that will kick out of my loop before it should. So is adding a
return
statement like so:

...
for (var i = 0; i < thingList.length; i++) {
if (thingList[i].id === req.params.somethingId) {
thingList.splice(i, 1);
return res.send(thingList[i]);
}
}
res.send("Nothing with that ID found");
...


or, alternatively:

...
res.send(thingList[i]);
return;
...


the best options I have? I'm teaching this to some people and wanted to make sure I had it right before giving them a solution that might be considered hacky.

Answer

Returning callbacks in Node is a common convention -- so yes, that is one option.

Another simple option is to store your match in a variable if found and break from your loop.

var match;
for (var i = 0; i < thingList.length; i++) {
    if (thingList[i].id === req.params.somethingId) {
        match = thingList[i];
        break;
    }
}
if(match !== undefined) {
    res.send(match);
} else {
    res.send("Nothing with that ID found");
}
Comments