0

I am very new to Node; I checked Error: Can't set headers after they are sent to the client but did not understand some terminologies.

Purpose of code: URL shortener post method

I got this error as a result of trying to post a URL that has an undefined DNS address to MongoDB (For example, https://www.iuhwefiudygkufge.com passes the regex test for https but the error below appears)

Schema and Model:

var urlSchema = new mongoose.Schema({
  url: String,
  shortUrl: String
});
var Url = mongoose.model("Url", urlSchema);

Code for post handler:

app.post("/api/shorturl/new", function(req, res) {
  var url = req.body.url;
  console.log(url);
  var httpRegex = /^(https?:\/\/)/;
  if (!httpRegex.test(url)) {
    res.json({ error: "invalid url" });
    return;
  }
  if (/^(https:\/\/)/.test(url)) {
    var httpsDroppedUrl = url.slice(8);
    dns.lookup(httpsDroppedUrl, function(err, address, family) {
      console.log("address: %j family: IPv%s", address, family);
      if (address == undefined) {
        res.json({ error: "invalid url" });
        return;
      }
    });
  } else {
    var httpDroppedUrl = url.slice(7);
    dns.lookup(httpDroppedUrl, function(err, address, family) {
      console.log("address: %j family: IPv%s", address, family);
      if (address == undefined) {
        res.json({ error: "invalid url" });
        return;
      }
    });
  }

  var newUrl =
    (Math.floor(Math.sqrt(url.length)) * 3).toString() +
    +Math.floor(Math.random() * 9999).toString() +
    Math.floor(Math.random() * 500).toString(); //random method to generate unique identifier
  console.log(newUrl);

  var data = new Url({
    url: url,
    shortUrl: newUrl
  });

  data.save(function(err) {
    if (err) {
      res.send("Error; please try again");
    }
  });

  res.json(data);
});

Here is the complete error I am getting:

_http_outgoing.js:467
    throw new ERR_HTTP_HEADERS_SENT('set');
    ^
Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
    at ServerResponse.setHeader (_http_outgoing.js:467:11)
    at ServerResponse.header (/rbd/pnpm-volume/10bcf34e-5673-48ca-b962-5eaef435922b/node_modules/.registry.npmjs.org/express/4.17.1/node_modules/express/lib/response.js:771:10)
    at ServerResponse.send (/rbd/pnpm-volume/10bcf34e-5673-48ca-b962-5eaef435922b/node_modules/.registry.npmjs.org/express/4.17.1/node_modules/express/lib/response.js:170:12)
    at ServerResponse.json (/rbd/pnpm-volume/10bcf34e-5673-48ca-b962-5eaef435922b/node_modules/.registry.npmjs.org/express/4.17.1/node_modules/express/lib/response.js:267:15)
at /app/server.js:58:13
    at processTicksAndRejections (internal/process/task_queues.js:83:17)

EDIT 1: next() replaced with return

I'm not sure where my code is going wrong

electro
  • 11
  • 3
  • When you use `res.json` you don't need to call `next()` because the `request-response` cycle is ended. `next()` is used when you want your request content to go through multiple middlewares between the request and the response. – BlackMath Nov 25 '20 at 04:05
  • I tried removing all the next() calls, but it has the same behavior. For some reason, ```var httpsDroppedUrl = url.slice(8);``` is executed directly before ```var newUrl = (Math.floor(Math.sqrt(url.length)) * 3).toString() + +Math.floor(Math.random() * 9999).toString() + Math.floor(Math.random() * 500).toString(); console.log(newUrl);``` and then it does the callback inside dns.lookup. Why is it jumping from the httpsDroppedUrl to the newUrl and then back to the dns.lookup? – electro Nov 25 '20 at 05:00
  • I suspect that `lookup` is a asynchronous function, which means that it is being executing in the background until it gets the result, while the rest of the code that doesn't depend on it continue executing. I agree with Anatoly's answer below. When you have multiple `res.json`, you need to put `return` before them, except for the last one. – BlackMath Nov 25 '20 at 13:24
  • Is there a way to delay the execution of the remaining code until after the asynchronous function is done executing? – electro Nov 25 '20 at 17:18

1 Answers1

0

You need to replace all next() calls with return because:

  1. next() is not needed after res.send/json - they already set a status and send a response to a client.
  2. add return after each res.send/json inside if's because calling res.send/json does not return you immediately from this handler

Also you need to turn dns.lookup calls into async functions using promises because right now they don't wait a result and execution continues immediately.

function dnsLookup(url) {
  return new Promise((resolve, reject) => {
    dns.lookup(url, function(err, address, family) {
      console.log("address: %j family: IPv%s", address, family);
      if (address == undefined) {
        // you can call reject(new Error("invalid url") here and catch it in outer function
        resolve({ error: "invalid url" });
        return;
      }
      resolve();
    });
  }
}
if (/^(https:\/\/)/.test(url)) {
    var httpsDroppedUrl = url.slice(8);
    const lookupResult = await dnsLookup(httpsDroppedUrl);
    if (lookupResult.error) {
      res.json({ error: lookupResult.error });
      return;
    }
}
Anatoly
  • 20,799
  • 3
  • 28
  • 42
  • I replaced each ```next()``` with ```return``` but I am still facing the issue I had before. – electro Nov 25 '20 at 17:19
  • can you update your post with the most recent code? – Anatoly Nov 25 '20 at 17:24
  • I just did that – electro Nov 25 '20 at 21:18
  • Ok, I see another issue: `dns.lookup` returns immediately because its continuation is in a passed callback. Maybe it will be better to turn the whole request handler to async and wrap all `dns.lookup` call into a function with `new Promise`. That way code will work pretty straight forward and more like you planned it should work. – Anatoly Nov 25 '20 at 21:27