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.

Sign up for GitHub

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

Jump to bottom

Fix issues with node 0.12 #73

Merged
merged 3 commits into from
Mar 4, 2015
Merged

Fix issues with node 0.12 #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
12011cd
Handle missing abort() response in node 0.12
kanongil Feb 23, 2015
3e13456
Use more compatible SSL method
kanongil Feb 23, 2015
865900c
Test on 0.12 & io.js
kanongil Feb 23, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
language: node_js

node_js:
- 0.10
- "0.10"
- "0.12"
- iojs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might as well add iojs

25 changes: 21 additions & 4 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,6 @@ exports.request = function (method, url, options, callback, _trace) {
var finish = function (err, res) {

if (!callback || err) {
if (res) {
res.destroy();
}

req.abort();
}

Expand Down Expand Up @@ -177,6 +173,27 @@ exports.request = function (method, url, options, callback, _trace) {
req.write(options.payload);
}

// Custom abort method to detect early aborts

var _abort = req.abort;
var aborted = false;
req.abort = function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a fan of changing the node req object. Can you find another way to solve this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not, as far as I can tell. This can only be resolved in node itself. You are welcome to submit a bug report.

As it stands, if abort() is called before the socket has been assigned, node >= 0.12 will silently drop the request without any further emits on the request object. This is all handled in https://github.com/joyent/node/blob/master/lib/_http_client.js.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who else can call abort() other than line 87? Because is no one, then why override the node internals?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

abort() is part of the exposed api in the req object that is returned. It should definitely be supported. Without this patch an early abort() call can cause the request callback to never be called.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what is the use case? It's one thing to protect the wreck internals but I don't think wreck users should be calling abort.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is more of a question for node. It is part of the public api, and I guess they have a reason for it to be there. See http://nodejs.org/api/http.html#http_request_abort. It is inherently also part of the wreck api, with test cases to support it. Also, there is no way to remove this.

My primary concern is that without this (or similar) patch, users can experience obscure timing related bugs when using abort. It is quite bad form to have a callback that is not always called.

I definitely agree that it sucks to do this, and I ultimately think that this should be solved in node. Until then I think we have 3 options:

  1. Use this patch
  2. Drop the abort tests & document divergent abort behavior
  3. Drop 0.12 support

Finally, there is a small possibility that I'm wrong about the missing notification and it can be solved without a hack. You are welcome to look for it.


if (!aborted && !req.res && !req.socket) {
process.nextTick(function () {

// Fake an ECONNRESET error

var error = new Error('socket hang up');
error.code = 'ECONNRESET';
finish(error);
});
}

aborted = true;
return _abort.call(req);
};

// Finalize request

req.end();
Expand Down
24 changes: 23 additions & 1 deletion test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ describe('request()', function () {

it('requests an https resource with secure protocol set', function (done) {

Wreck.request('get', 'https://google.com', { rejectUnauthorized: true, secureProtocol: 'SSLv3_method' }, function (err, res) {
Wreck.request('get', 'https://google.com', { rejectUnauthorized: true, secureProtocol: 'SSLv23_method' }, function (err, res) {

expect(err).to.not.exist();
Wreck.read(res, null, function (err, body) {
Expand Down Expand Up @@ -667,6 +667,28 @@ describe('request()', function () {
});
});

it('in-progress requests can be aborted', function (done) {

var wreck;
var server = Http.createServer(function (req, res) {

res.writeHead(200);
res.end();

wreck.abort();
});

server.listen(0, function () {

wreck = Wreck.request('get', 'http://localhost:' + server.address().port, {}, function (err) {

expect(err).to.exist();
expect(err.code).to.equal('ECONNRESET');
done();
});
});
});

it('uses agent option', function (done) {

var agent = new Http.Agent();
Expand Down