Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Domain exceptions within a http.request callback #4702

Closed
trevorsheridan opened this Issue · 12 comments

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

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
@trevorsheridan

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

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

@trevorsheridan

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
Owner

@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

@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
Owner

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
Owner

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

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
Closed

Domain exceptions within a riak-js callback #150

@trevorsheridan

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

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

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 join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.