This repository has been archived by the owner. It is now read-only.

Domain exceptions within a http.request callback #4702

Closed
trevorsheridan opened this Issue Feb 1, 2013 · 12 comments

Comments

Projects
None yet
3 participants
@trevorsheridan

I cannot seem to figure out why exceptions from within an http.request callback cause the callback to no longer get called after 5 requests. The following code illustrates this issue:

var domain = require("domain"),
  http = require("http");

for (var i = 0; i < 10; i++) {
  var d = domain.create();
  d.run(function() {
    var request = http.request({ hostname: "google.com", port: 80, method: "HEAD" }, function(response) {
      throw "An exception from the response handler";
    });
    request.on("error", function(error) {
      console.log("request caught " + error);
    });
    request.end();
  });

  d.on("error", function(error) {
    console.log("domain caught " + error);
  });
}
@bnoordhuis

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Feb 1, 2013

Member

You are using the default http.Agent object, http.globalAgent, which has maxSockets=5. If you increase maxSockets or give each request its own agent, all 10 requests will be made.

In order for your example to work properly with domains, you have to rewrite it to something like this:

var domain = require("domain"),
    http = require("http");

for (var i = 0; i < 10; i++) makeRequest();

function makeRequest()
{
  var options = {
    agent: new http.Agent,
    hostname: "google.com",
    port: 80,
    method: "HEAD"
  };
  var request = http.request(options, function(response) {
    throw new Error("An exception from the response handler");
  });
  request.on("error", function(error) {
    console.log("request caught " + error);
  });
  request.end();

  var d = domain.create();
  d.add(request);
  d.add(options.agent);
  d.on("error", function(error) {
    console.log("domain caught " + error);
    d.dispose();
  });
}

(Note the calls to Domain#add.)

Member

bnoordhuis commented Feb 1, 2013

You are using the default http.Agent object, http.globalAgent, which has maxSockets=5. If you increase maxSockets or give each request its own agent, all 10 requests will be made.

In order for your example to work properly with domains, you have to rewrite it to something like this:

var domain = require("domain"),
    http = require("http");

for (var i = 0; i < 10; i++) makeRequest();

function makeRequest()
{
  var options = {
    agent: new http.Agent,
    hostname: "google.com",
    port: 80,
    method: "HEAD"
  };
  var request = http.request(options, function(response) {
    throw new Error("An exception from the response handler");
  });
  request.on("error", function(error) {
    console.log("request caught " + error);
  });
  request.end();

  var d = domain.create();
  d.add(request);
  d.add(options.agent);
  d.on("error", function(error) {
    console.log("domain caught " + error);
    d.dispose();
  });
}

(Note the calls to Domain#add.)

@bnoordhuis bnoordhuis closed this Feb 1, 2013

@trevorsheridan

This comment has been minimized.

Show comment Hide comment
@trevorsheridan

trevorsheridan Feb 1, 2013

So is the problem that I'm running up against was due to the fact that I was throwing an exception within the callback which prevented the agent from purging closed sockets?

So is the problem that I'm running up against was due to the fact that I was throwing an exception within the callback which prevented the agent from purging closed sockets?

@bnoordhuis

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Feb 1, 2013

Member

Not exactly. You didn't dispose the domain after it caught the exception.

Member

bnoordhuis commented Feb 1, 2013

Not exactly. You didn't dispose the domain after it caught the exception.

@trevorsheridan

This comment has been minimized.

Show comment Hide comment
@trevorsheridan

trevorsheridan Feb 1, 2013

Ah that makes sense!

So my next question is why doesn't the implicit binding work in this case? The following two code examples are based on yours and mine. The first (based on my example) uses .run for implicit binding:

var domain = require("domain"),
    http = require("http");

for (var i = 0; i < 10; i++) makeRequest();

function makeRequest() {
  var d = domain.create();
  d.run(function() {
    var request = http.request({ hostname: "google.com", port: 80, method: "HEAD" }, function(response) {
      throw "An exception from the response handler";
    });
    request.end();
  });

  d.on("error", function(error) {
    console.log("domain caught " + error);
    d.dispose();
  });
}

The second (based on your example) uses .add for explicit binding. This example doesn't make use of a new agent either, it uses the global agent and works as expected:

var domain = require("domain"),
    http = require("http");

for (var i = 0; i < 10; i++) makeRequest();

function makeRequest() {
  var request = http.request({ hostname: "google.com", port: 80, method: "HEAD" }, function(response) {
    throw "An exception from the response handler";
  });
  request.end();

  var d = domain.create();
  d.add(request);
  d.on("error", function(error) {
    console.log("domain caught " + error);
    d.dispose();
  });
}

Ah that makes sense!

So my next question is why doesn't the implicit binding work in this case? The following two code examples are based on yours and mine. The first (based on my example) uses .run for implicit binding:

var domain = require("domain"),
    http = require("http");

for (var i = 0; i < 10; i++) makeRequest();

function makeRequest() {
  var d = domain.create();
  d.run(function() {
    var request = http.request({ hostname: "google.com", port: 80, method: "HEAD" }, function(response) {
      throw "An exception from the response handler";
    });
    request.end();
  });

  d.on("error", function(error) {
    console.log("domain caught " + error);
    d.dispose();
  });
}

The second (based on your example) uses .add for explicit binding. This example doesn't make use of a new agent either, it uses the global agent and works as expected:

var domain = require("domain"),
    http = require("http");

for (var i = 0; i < 10; i++) makeRequest();

function makeRequest() {
  var request = http.request({ hostname: "google.com", port: 80, method: "HEAD" }, function(response) {
    throw "An exception from the response handler";
  });
  request.end();

  var d = domain.create();
  d.add(request);
  d.on("error", function(error) {
    console.log("domain caught " + error);
    d.dispose();
  });
}
@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Feb 1, 2013

@bnoordhuis domain.dispose doesn't do what it looks like it does. (It's a bad API, and should be removed, imo.) It's not necessary, and not reliable.

You also don't have to explicitly add the req object, since it's created in the scope of the domain, and will have an implicit link to the domain object. d.run(function() { e = new EventEmitter() }) is effectively the same as e = new EventEmitter(); d.add(e);

isaacs commented Feb 1, 2013

@bnoordhuis domain.dispose doesn't do what it looks like it does. (It's a bad API, and should be removed, imo.) It's not necessary, and not reliable.

You also don't have to explicitly add the req object, since it's created in the scope of the domain, and will have an implicit link to the domain object. d.run(function() { e = new EventEmitter() }) is effectively the same as e = new EventEmitter(); d.add(e);

@trevorsheridan

This comment has been minimized.

Show comment Hide comment
@trevorsheridan

trevorsheridan Feb 1, 2013

@isaacs that's exactly what I thought as well regarding the implicit behavior.

The example below is what makes me feel like the agent isn't freeing up the sockets and processing new requests from the requests queue. I can't figure out what is causing this behavior though.

If you comment out the http.globalAgent.sockets["google.com:80"][0].end() call you will see the original issue and see that both sockets and requests have 5 objects. This happens for a few minutes until something expires, then the agent processes the remaining 5 requests from the queue. Comment back in that line of code and you will see it emulating what should be happening.

I can't figure out what it's getting hung up on within the domain.

var domain = require("domain"),
  http = require("http");

for (var i = 0; i < 10; i++) makeRequest();

function makeRequest() {
  var d = domain.create();

  d.run(function() {
    var request = http.request({ hostname: "google.com", port: 80, method: "HEAD" }, function(response) {
      throw "An exception from the response handler";
    });
    request.end();
  });

  d.on("error", function(error) {
    console.log("domain caught " + error);
    d.dispose();
  });
}

setInterval(function() {
  if (http.globalAgent.sockets["google.com:80"]) {
    console.log(http.globalAgent);
    http.globalAgent.sockets["google.com:80"][0].end();
  }
}, 500);

@isaacs that's exactly what I thought as well regarding the implicit behavior.

The example below is what makes me feel like the agent isn't freeing up the sockets and processing new requests from the requests queue. I can't figure out what is causing this behavior though.

If you comment out the http.globalAgent.sockets["google.com:80"][0].end() call you will see the original issue and see that both sockets and requests have 5 objects. This happens for a few minutes until something expires, then the agent processes the remaining 5 requests from the queue. Comment back in that line of code and you will see it emulating what should be happening.

I can't figure out what it's getting hung up on within the domain.

var domain = require("domain"),
  http = require("http");

for (var i = 0; i < 10; i++) makeRequest();

function makeRequest() {
  var d = domain.create();

  d.run(function() {
    var request = http.request({ hostname: "google.com", port: 80, method: "HEAD" }, function(response) {
      throw "An exception from the response handler";
    });
    request.end();
  });

  d.on("error", function(error) {
    console.log("domain caught " + error);
    d.dispose();
  });
}

setInterval(function() {
  if (http.globalAgent.sockets["google.com:80"]) {
    console.log(http.globalAgent);
    http.globalAgent.sockets["google.com:80"][0].end();
  }
}, 500);
@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Feb 2, 2013

Ah, so, the way that you have it set up here, there's no way to destroy the socket (or request/response) when the error happens, because you don't have access to it.

d.add(request) and d.dispose() actually WOULD do that, because that's what dispose() does: it tries to destroy anything that has a destroy() method. (Brutally ugly, I know. It also calls end and close and whatever other no moar! type methods it can find.)

Try this:

for (var i = 0; i < 10; i++) makeRequest();

function makeRequest() {

  var d = domain.create();

  var request;
  d.run(function() {
    request = http.request({ hostname: "google.com", port: 80, method: "HEAD" }, function(response) {
      throw new Error("An exception from the response handler");
    });
    request.end();
  });

  d.on("error", function(error) {
    console.log("domain caught " + error);
    request.destroy();
  });
}

isaacs commented Feb 2, 2013

Ah, so, the way that you have it set up here, there's no way to destroy the socket (or request/response) when the error happens, because you don't have access to it.

d.add(request) and d.dispose() actually WOULD do that, because that's what dispose() does: it tries to destroy anything that has a destroy() method. (Brutally ugly, I know. It also calls end and close and whatever other no moar! type methods it can find.)

Try this:

for (var i = 0; i < 10; i++) makeRequest();

function makeRequest() {

  var d = domain.create();

  var request;
  d.run(function() {
    request = http.request({ hostname: "google.com", port: 80, method: "HEAD" }, function(response) {
      throw new Error("An exception from the response handler");
    });
    request.end();
  });

  d.on("error", function(error) {
    console.log("domain caught " + error);
    request.destroy();
  });
}
@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Feb 2, 2013

You could also do it this way:

var domain = require("domain"),
  http = require("http");

for (var i = 0; i < 10; i++) makeRequest();

function makeRequest() {

  var d = domain.create();

  d.run(function() {
    var request = http.request({ hostname: "google.com", port: 80, method: "HEAD" }, function(response) {
      throw new Error("An exception from the response handler");
    });
    request.end();

    d.on("error", function(error) {
      console.log("domain caught " + error);
      request.destroy();
    });
  });
}

isaacs commented Feb 2, 2013

You could also do it this way:

var domain = require("domain"),
  http = require("http");

for (var i = 0; i < 10; i++) makeRequest();

function makeRequest() {

  var d = domain.create();

  d.run(function() {
    var request = http.request({ hostname: "google.com", port: 80, method: "HEAD" }, function(response) {
      throw new Error("An exception from the response handler");
    });
    request.end();

    d.on("error", function(error) {
      console.log("domain caught " + error);
      request.destroy();
    });
  });
}
@trevorsheridan

This comment has been minimized.

Show comment Hide comment
@trevorsheridan

trevorsheridan Feb 2, 2013

Thanks @isaacs that makes sense!

My problem is a little bigger than this example shows because the issue is happening from within another library thats making the request. I don't know how to solve this issue since the request is being made from within another library where I have no access to the request object.

Thanks @isaacs that makes sense!

My problem is a little bigger than this example shows because the issue is happening from within another library thats making the request. I don't know how to solve this issue since the request is being made from within another library where I have no access to the request object.

@trevorsheridan trevorsheridan referenced this issue in mostlyserious/riak-js Feb 2, 2013

Closed

Domain exceptions within a riak-js callback #150

@trevorsheridan

This comment has been minimized.

Show comment Hide comment
@trevorsheridan

trevorsheridan Feb 3, 2013

After some digging I think I understand the problem. I think it relates to the node_http_parser/http_parser_execute callback mechanism. When the exception is thrown within the callback the rest of socketOnData (http.js) is unable to continue executing and unable to destroy the socket. The socket is then left hanging until the keepalive timer expires. If a socket's keepalive is set to false this issue is nonexistent.

After some digging I think I understand the problem. I think it relates to the node_http_parser/http_parser_execute callback mechanism. When the exception is thrown within the callback the rest of socketOnData (http.js) is unable to continue executing and unable to destroy the socket. The socket is then left hanging until the keepalive timer expires. If a socket's keepalive is set to false this issue is nonexistent.

@trevorsheridan

This comment has been minimized.

Show comment Hide comment
@trevorsheridan

trevorsheridan Feb 3, 2013

Here's my workaround for this issue. Since I have complete control over my callbacks I'm simply going to wrap them in a function that performs it's own try/catch. If it catches an error it will emit the error to the active domain.

var domain = require("domain"),
  http = require("http");

Object.defineProperty(Function.prototype, "callback", {
  value: function() {
    return function() {
      try {
        return this.apply(this, Array.prototype.slice.call(arguments));
      } catch(error) {
        /* Rethrow the error if there isn't an active domain. If an 'uncaughtException'
        listener is enabled this will yield the same "socket hanging" issue... but
        you shouldn't be using process.on('uncaughtException') anyways. If there
        is no 'uncaughtException' listener, then the process will simply exit and
        behave the exact same way as exceptions within callbacks do today.
        */
        if (!domain.active)
          throw error;

        domain.active.emit("error", error);
        return false;
      }
    }.bind(this);
  }
});

for (var i = 0; i < 10; i++) makeRequest();

function makeRequest() {
  var d = domain.create();

  d.run(function() {
    var request = http.request({ hostname: "google.com", port: 80, method: "HEAD" }, function(response) {
      throw new Error("An exception from the response handler");
    }.callback().bind(this));
    request.end();

    d.on("error", function(error) {
      console.log("domain caught " + error);
      d.dispose();
    });
  });
}

Here's my workaround for this issue. Since I have complete control over my callbacks I'm simply going to wrap them in a function that performs it's own try/catch. If it catches an error it will emit the error to the active domain.

var domain = require("domain"),
  http = require("http");

Object.defineProperty(Function.prototype, "callback", {
  value: function() {
    return function() {
      try {
        return this.apply(this, Array.prototype.slice.call(arguments));
      } catch(error) {
        /* Rethrow the error if there isn't an active domain. If an 'uncaughtException'
        listener is enabled this will yield the same "socket hanging" issue... but
        you shouldn't be using process.on('uncaughtException') anyways. If there
        is no 'uncaughtException' listener, then the process will simply exit and
        behave the exact same way as exceptions within callbacks do today.
        */
        if (!domain.active)
          throw error;

        domain.active.emit("error", error);
        return false;
      }
    }.bind(this);
  }
});

for (var i = 0; i < 10; i++) makeRequest();

function makeRequest() {
  var d = domain.create();

  d.run(function() {
    var request = http.request({ hostname: "google.com", port: 80, method: "HEAD" }, function(response) {
      throw new Error("An exception from the response handler");
    }.callback().bind(this));
    request.end();

    d.on("error", function(error) {
      console.log("domain caught " + error);
      d.dispose();
    });
  });
}
@trevorsheridan

This comment has been minimized.

Show comment Hide comment
@trevorsheridan

trevorsheridan Feb 3, 2013

I turned this bit of code into a small utility. Hopefully it will help others that run into this issue.

https://github.com/trevorsheridan/callable

I turned this bit of code into a small utility. Hopefully it will help others that run into this issue.

https://github.com/trevorsheridan/callable

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.