This repository was archived by the owner on Apr 22, 2023. It is now read-only.
Repository navigation
This repository was archived by the owner on Apr 22, 2023. It is now read-only.
http: add abort event request() #9278
Description
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();
});
Activity
http: emit abort event from ClientRequest
cjihrig commentedon Feb 25, 2015
@misterdjules I propose landing nodejs/node@2ca22aa to address this. Thoughts?
geek commentedon Feb 26, 2015
@cjihrig and @misterdjules seems like a good addition to node
misterdjules commentedon Feb 26, 2015
@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 commentedon Feb 27, 2015
@misterdjules if you want the back story, see hapijs/wreck#73. Basically, a library passes an instance of
ClientRequestaround. 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 commentedon Feb 27, 2015
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
abortto 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 commentedon Feb 27, 2015
@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 commentedon Feb 27, 2015
@cjihrig ya, I think my preference would be
if(this.aborted) return;if multiple calls toabort()should be allowed to succeed.michaelnisi commentedon Mar 3, 2015
Why isn't this covered with an
'error'event? It's unclear to me why it should be so specific.kanongil commentedon Mar 3, 2015
@michaelnisi Indeed. I had a similar concern towards the io.js issue.
cjihrig commentedon Mar 3, 2015
Explicitly aborting a connection is not an error.
kanongil commentedon Mar 3, 2015
@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 commentedon Mar 3, 2015
@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 commentedon Mar 3, 2015
I don't question adding an
'abort'event, but removing the'error'event.cjihrig commentedon Nov 22, 2015
Closing as 0.13 seems unlikely to be released.