Reputation:
I have a nodejs app which uses express. When I try to call this method below I get logged in console "Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client".
I found some other threads here about it being the async-methods, but I cant seem to pinpoint where its wrong in my code. I tried console logging, and the last place that logs before the return is just after this try catch. I also added "Return" before sending res.status to all my code, but it still emits this error. The code in the controller is much longer, but no console.logs where made after this piece of code. Any idas what Im doing wrong?
router.post('/add', auth, async (req, res) => {
if(req.body.mobilenumber === ''){
return res.status(400).send('Bad input data, please check input.')
}
const checkUserText = 'SELECT * FROM SQL';
let dbResult1;
try {
dbResult1 = await GetStuff(checkUserText, [req.body.mobilenumber]);
} catch (err) {
logger.error(err)
return res.status(500).send(err)
}
if (dbResult1.rowCount === null || dbResult1.rowCount >= 1) {
return res.sendStatus(405).send('User already exist')
}
var insertUser = 'INSERT INTO SQL RETURNING *';
let dbResult2;
try {
let insData = [paramsFromBody];
dbResult2= await GetStuff(insertUser, insData);
} catch (err) {
logger.error(err)
return res.status(500).send(err)
}
if (dbResult2 === null || dbResult2.rowCount === 0) {
return res.status(500).send('error')
logger.error('Error')
}
return res.status(200).send('Added OK.')
})
async function GetStuff(text, id){
try {
return await db.query(text, id);
} catch(error) {
throw new Error('Failed DB-action' + error);
}
}
Upvotes: 0
Views: 215
Reputation: 707148
From what I see in the code you've shared, this will cause the error you report:
return res.sendStatus(405).send('User already exist')
because .sendStatus()
sends the whole response immediately and then .send()
tries to send another response. That should be this:
return res.status(405).send('User already exist');
Also, you can simplify GetStuff()
to just this:
function GetStuff(text, id) {
return db.query(text, id).catch(err => {
throw new Error('Failed DB-action' + err);
});
}
One clue is that there's pretty much never a reason to do:
return await fn();
You can just do:
return fn();
Both return the same promise with the same resolved value.
Also, note that in this code:
if (dbResult2 === null || dbResult2.rowCount === 0) {
return res.status(500).send('error')
logger.error('Error')
}
The logger.error()
will never get called since it's after a return
statement.
Your code could be simplified by using one central try/catch and only sending the response in two places (one for success and one for error):
// our own error subclass that holds a status value
class ResponseError extends Error {
constructor(msg, status) {
super(msg);
this.status = status;
}
}
router.post('/add', auth, async (req, res) => {
try {
if (req.body.mobilenumber === '') {
throw new ResponseError('Bad input data, please check input.', 400);
}
const checkUserText = 'SELECT * FROM SQL';
let dbResult1 = await GetStuff(checkUserText, [req.body.mobilenumber]);
if (dbResult1.rowCount === null || dbResult1.rowCount >= 1) {
throw new ResponseError('User already exist', 405);
}
const insertUser = 'INSERT INTO SQL RETURNING *';
let dbResult2 = await GetStuff(insertUser, [paramsFromBody]);
if (dbResult2 === null || dbResult2.rowCount === 0) {
throw new ResponseError("dbResult2 empty", 500);
}
res.send('Added OK.')
} catch(e) {
// if no specific status specified, use 500
let status = e.status || 500;
res.status(status).send(e);
}
})
function GetStuff(text, id) {
return db.query(text, id).catch(err => {
throw new Error('Failed DB-action' + error);
});
}
Upvotes: 2