-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
http: emit abort event from ClientRequest #945
Conversation
@evanlucas this is what I had in mind, and this LGTM. However, would you be opposed to the following code as the test: var assert = require('assert');
var http = require('http');
var common = require('../common');
var server = http.createServer(function(req, res) {
res.end();
});
server.listen(common.PORT, function() {
var req = http.request({
port: common.PORT
}, function() {
assert(false, 'should not receive data');
});
req.on('abort', function() {
server.close();
});
req.end();
req.abort();
}); |
+1 |
So from now on I will have to listen to |
@kanongil IIRC isn't |
@mscdex Maybe, but the new abort logic can prevent the request from ever being assigned a socket. |
a3953a1
to
6ee626b
Compare
@cjihrig works for me. Updated :] |
|
||
process.nextTick(function() { | ||
self.emit('abort'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we gate this so that abort
can only be emitted once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should. Fixing it now.
LGTM, other than the gating question. |
ClientRequest will now emit an `abort` event to detect when `abort` is called.
6ee626b
to
ea0fb7c
Compare
ok, updated so that |
LGTM. |
ClientRequest will now emit an abort event the first time abort() is called. Semver: Minor Fixes: nodejs/node-v0.x-archive#9278 PR-URL: #945 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Thanks! Landed in 2ca22aa. I added a note to the docs stating that |
Notable changes: * process / promises: An'unhandledRejection' event is now emitted on process whenever a Promise is rejected and no error handler is attached to the Promise within a turn of the event loop. A 'rejectionHandled' event is now emitted whenever a Promise was rejected and an error handler was attached to it later than after an event loop turn. See the process documentation for more detail. #758 (Petka Antonov) * streams: you can now use regular streams as an underlying socket for tls.connect() #758 (Fedor Indutny) * http: A new 'abort' event emitted when a http.ClientRequest is aborted by the client. #945 (Evan Lucas) * V8: Upgrade V8 to 4.1.0.21. Includes an embargoed fix, details should be available at https://code.google.com/p/chromium/issues/detail?id=430201 when embargo is lifted. A breaking ABI change has been held back from this upgrade, possibly to be included when io.js merges V8 4.2. See #952 for discussion. * npm: Upgrade npm to 2.6.0. Includes features to support the new registry and to prepare for npm@3. See npm CHANGELOG.md https://github.com/npm/npm/blob/master/CHANGELOG.md#v260-2015-02-12 for details. * libuv: Upgrade to 1.4.2. See libuv ChangeLog https://github.com/libuv/libuv/blob/v1.x/ChangeLog for details of fixes.
ClientRequest will now emit an
abort
event to detect whenabort
iscalled.
Ref: nodejs/node-v0.x-archive#9278