Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

evanlucas
Copy link
Contributor

ClientRequest will now emit an abort event to detect when abort is
called.

Ref: nodejs/node-v0.x-archive#9278

@cjihrig
Copy link
Contributor

cjihrig commented Feb 25, 2015

@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();
});

@brendanashworth
Copy link
Contributor

+1

@kanongil
Copy link
Contributor

So from now on I will have to listen to end, error, and abort to know when a request is "done"? Why not just fix the missing error emit regression?

@mscdex
Copy link
Contributor

mscdex commented Feb 25, 2015

@kanongil IIRC isn't close always emitted after end and error (for sockets anyway)? I'm assuming close would fire after abort too? I typically just listen for close to know when the socket/request/whatever is "done."

@kanongil
Copy link
Contributor

@mscdex Maybe, but the new abort logic can prevent the request from ever being assigned a socket.

@evanlucas evanlucas force-pushed the http-client-abort-event branch from a3953a1 to 6ee626b Compare February 25, 2015 16:32
@evanlucas
Copy link
Contributor Author

@cjihrig works for me. Updated :]

@cjihrig cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 25, 2015

process.nextTick(function() {
self.emit('abort');
});
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@chrisdickinson
Copy link
Contributor

LGTM, other than the gating question.

ClientRequest will now emit an `abort` event to detect when `abort` is
called.
@evanlucas evanlucas force-pushed the http-client-abort-event branch from 6ee626b to ea0fb7c Compare February 25, 2015 21:23
@evanlucas
Copy link
Contributor Author

ok, updated so that abort will only be emitted if it hasn't already been emitted

@chrisdickinson
Copy link
Contributor

LGTM.

evanlucas added a commit that referenced this pull request Feb 25, 2015
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 <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
@cjihrig
Copy link
Contributor

cjihrig commented Feb 25, 2015

Thanks! Landed in 2ca22aa. I added a note to the docs stating that abort would only be emitted on the first call to abort(). I floated in the current libuv patch locally to run the tests (but did not commit it).

@cjihrig cjihrig closed this Feb 25, 2015
@rvagg rvagg mentioned this pull request Feb 26, 2015
rvagg added a commit that referenced this pull request Feb 26, 2015
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants