http: don't pass the request options to Agent

The `options` that were being passed in before here are specific to a
single request, which kinda defeats the purpose of using an Agent in the
first place.

On a worse note, these `options` have not yet been "processed" by the
`http.ClientRequest` class, so if `port: null` is set (like it is as the
result of a `url.parse()` call), then they take preference over the
processed values since the agent's "options" get mixed in last in the
`createSocket()` function.

Fixes #6197.
Fixes #6199.
Closes #6231.

diff --git a/lib/_http_agent.js b/lib/_http_agent.js
index 7fa818f..2c57829 100644 (file)
--- a/lib/_http_agent.js
+++ b/lib/_http_agent.js
@@ -287,7 +287,7 @@ Agent.prototype.request = function(options, cb) {
 
   // if it's false, then make a new one, just like this one.
   if (options.agent === false)
-    options.agent = new this.constructor(options);
+    options.agent = new this.constructor();
 
   debug('agent.request', options);
   return new ClientRequest(options, cb);

diff --git a/test/simple/test-http-agent-false.js b/test/simple/test-http-agent-false.js
new file mode 100644 (file)
index 0000000..d897ab6
--- /dev/null
+++ b/test/simple/test-http-agent-false.js
@@ -0,0 +1,53 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+var http = require('http');
+
+// sending `agent: false` when `port: null` is also passed in (i.e. the result of
+// a `url.parse()` call with the default port used, 80 or 443), should not result
+// in an assertion error...
+var opts = {
+  host: '127.0.0.1',
+  port: null,
+  path: '/',
+  method: 'GET',
+  agent: false
+};
+
+var good = false;
+process.on('exit', function() {
+  assert(good, 'expected either an "error" or "response" event');
+});
+
+// we just want an "error" (no local HTTP server on port 80) or "response"
+// to happen (user happens ot have HTTP server running on port 80). As long as the
+// process doesn't crash from a C++ assertion then we're good.
+var req = http.request(opts);
+req.on('response', function(res) {
+  good = true;
+});
+req.on('error', function(err) {
+  // an "error" event is ok, don't crash the process
+  good = true;
+});
+req.end();