http: add abort event request()

[cjihrig]

There doesn't seem to be a good way (or at least I haven't found one) to detect when ClientRequest.prototype.abort() is called. There is ClientRequest.prototype.aborted, but that would require observing the object. It would be nice to have an abort event emitted in this case.

As a test case, I would like to detect the abort in the following code:

var assert = require('assert');
var http = require('http');
var server = http.createServer(function(req, res) {
  res.end();
});

server.listen(4000, function() {
  function connect(cb) {
    var req = http.request({
      port: 4000
    }, cb);

    return req;
  }

  var req = connect(function() {
    assert(false);
  });


  req.abort();
});

[cjihrig]

@misterdjules I propose landing nodejs/node@2ca22aa to address this. Thoughts?

[geek]

@cjihrig and @misterdjules seems like a good addition to node

[misterdjules]

@cjihrig @geek I think I don't understand the use case correctly. Is it specific to when a request is aborted before the connection has been established?

[cjihrig]

@misterdjules if you want the back story, see hapijs/wreck#73. Basically, a library passes an instance of ClientRequest around. If the request is constructed and then immediately aborted, there doesn't appear to be a way of triggering a callback. I brought it up on today's call (I think before you joined). TJ mentioned that it should be fine for the master branch.

[tjfontaine]

It's totally reasonable to emit an event for 'abort', though it's an API addition and therefore not something that can be included in v0.12.

Looking at the change @cjihrig does it make sense in the scheme of things for abort to be called more than once? If the API is idempotent then it should have an early return, and avoid the rest of the method body. That should make the emit logic much more straight forward to only happen once, and preserve the time at which abort was called.

[cjihrig]

@tjfontaine do you mean effectively adding an else return; after this line (it could be rewritten in other ways)? If so, that makes sense to me.

[tjfontaine]

@cjihrig ya, I think my preference would be if(this.aborted) return; if multiple calls to abort() should be allowed to succeed.

[michaelnisi]

Why isn't this covered with an 'error' event? It's unclear to me why it should be so specific.

[kanongil]

@michaelnisi Indeed. I had a similar concern towards the io.js issue.

[cjihrig]

Explicitly aborting a connection is not an error.

[kanongil]

@cjihrig True, but it is the only decent way to signal the condition within the existing api, and it matches the 0.10 behavior. Note that this does not exclude adding an 'abort' event at a later stage.

[jasnell]

@kanongil ... while that would likely allow us to get this into v0.12 without introducing an api change, it seems like a short term fix. @cjihrig is right, aborting is not an error and shouldn't be treated as such. This is definitely a good change but it's something that can push to v0.13 (let's just not take a long time to get v0.13 out ;-) ...)

[michaelnisi]

I don't question adding an 'abort' event, but removing the 'error' event.

[cjihrig]

Closing as 0.13 seems unlikely to be released.