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: throw an error for unexpected agent values #10053

Closed
wants to merge 1 commit into
base: master
from

Conversation

@brad-decker
Contributor

brad-decker commented Dec 1, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http, test

Description of change

As per #9069 unexpected things can happen when supplying
an unexpected value to agent. Beings as the docs clearly
state the expected values, this throws an error on an
unexpected value.

Show outdated Hide outdated lib/_http_client.js
@@ -35,6 +34,13 @@ function ClientRequest(options, cb) {
} else if ((agent === null || agent === undefined) &&
typeof options.createConnection !== 'function') {
agent = defaultAgent;
} else if (agent !== null && agent !== undefined &&
!(agent instanceof Agent.Agent)) {
console.log(agent, typeof agent);

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 1, 2016

Member

Please remove the console.log.

@bnoordhuis

bnoordhuis Dec 1, 2016

Member

Please remove the console.log.

This comment has been minimized.

@brad-decker

brad-decker Dec 1, 2016

Contributor

doh, thats embarassing. Will remove.

@brad-decker

brad-decker Dec 1, 2016

Contributor

doh, thats embarassing. Will remove.

Show outdated Hide outdated lib/_http_client.js
@@ -35,6 +34,13 @@ function ClientRequest(options, cb) {
} else if ((agent === null || agent === undefined) &&
typeof options.createConnection !== 'function') {
agent = defaultAgent;
} else if (agent !== null && agent !== undefined &&
!(agent instanceof Agent.Agent)) {

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 1, 2016

Member

What is the reason for these specific checks vs. a simple } else {?

@bnoordhuis

bnoordhuis Dec 1, 2016

Member

What is the reason for these specific checks vs. a simple } else {?

This comment has been minimized.

@brad-decker

brad-decker Dec 1, 2016

Contributor

We should accept an instance of Agent.Agent. Had to check for the null value as well because the previous if statement also requires options.createConnection to not be a function.

This if block could probably be rewritten slightly to account for all acceptable values and then throw the error in a simple else clause.

@brad-decker

brad-decker Dec 1, 2016

Contributor

We should accept an instance of Agent.Agent. Had to check for the null value as well because the previous if statement also requires options.createConnection to not be a function.

This if block could probably be rewritten slightly to account for all acceptable values and then throw the error in a simple else clause.

Show outdated Hide outdated lib/_http_client.js
console.log(agent, typeof agent);
throw new Error(
'Agent option must be an instance of http.Agent, undefined or false. '
+ 'Received type: ' + typeof agent + ' instead'

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 1, 2016

Member

Tiny nit: can you indent by four spaces here? It's allowed to use string templates (backticks) if you think that's easier to read.

@bnoordhuis

bnoordhuis Dec 1, 2016

Member

Tiny nit: can you indent by four spaces here? It's allowed to use string templates (backticks) if you think that's easier to read.

This comment has been minimized.

@brad-decker

brad-decker Dec 1, 2016

Contributor

cool.

@brad-decker

brad-decker Dec 1, 2016

Contributor

cool.

This comment has been minimized.

@brad-decker

brad-decker Dec 2, 2016

Contributor

@cjihrig requested moving the + to the previous line, once doing that I got confused by the request for 4 spaces. Should the second line of text be indented another level?

@brad-decker

brad-decker Dec 2, 2016

Contributor

@cjihrig requested moving the + to the previous line, once doing that I got confused by the request for 4 spaces. Should the second line of text be indented another level?

Show outdated Hide outdated test/parallel/test-http-client-reject-unexpected-agent.js
method: 'GET',
port: undefined,
host: '127.0.0.1',
agent: true,

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 1, 2016

Member

Can you test some other values as well?

@bnoordhuis

bnoordhuis Dec 1, 2016

Member

Can you test some other values as well?

Show outdated Hide outdated test/parallel/test-http-client-reject-unexpected-agent.js
try {
http.request(options);
} catch (err) {

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 1, 2016

Member

Please use assert.throws() instead of try/catch. This should fail but won't when http.request doesn't throw an exception.

@bnoordhuis

bnoordhuis Dec 1, 2016

Member

Please use assert.throws() instead of try/catch. This should fail but won't when http.request doesn't throw an exception.

This comment has been minimized.

@brad-decker

brad-decker Dec 1, 2016

Contributor

👍 definitely

@brad-decker

brad-decker Dec 1, 2016

Contributor

👍 definitely

Show outdated Hide outdated test/parallel/test-http-client-reject-unexpected-agent.js
@@ -0,0 +1,21 @@
'use strict';
require('../common');
var assert = require('assert');

This comment has been minimized.

@cjihrig

cjihrig Dec 1, 2016

Contributor

Can you use const instead of var in this file.

@cjihrig

cjihrig Dec 1, 2016

Contributor

Can you use const instead of var in this file.

Show outdated Hide outdated test/parallel/test-http-client-reject-unexpected-agent.js
assert.strictEqual(
err.message,
'Agent option must be an instance of http.Agent, undefined or false. '
+ 'Received type: boolean instead'

This comment has been minimized.

@cjihrig

cjihrig Dec 1, 2016

Contributor

Can you move the + to the previous line.

@cjihrig

cjihrig Dec 1, 2016

Contributor

Can you move the + to the previous line.

@brad-decker

This comment has been minimized.

Show comment
Hide comment
@brad-decker

brad-decker Dec 2, 2016

Contributor

@cjihrig @bnoordhuis - Thanks for your feedback. I've addressed most of your remarks. I made some further changes to the logic inside of request.

Contributor

brad-decker commented Dec 2, 2016

@cjihrig @bnoordhuis - Thanks for your feedback. I've addressed most of your remarks. I made some further changes to the logic inside of request.

@brad-decker

This comment has been minimized.

Show comment
Hide comment
@brad-decker

brad-decker Dec 2, 2016

Contributor

Will squash the commits down prior to merge once approved and passing CI.

Contributor

brad-decker commented Dec 2, 2016

Will squash the commits down prior to merge once approved and passing CI.

Show outdated Hide outdated test/parallel/test-http-client-reject-unexpected-agent.js
res.end();
});
server.listen(0, baseOptions.host, function() {

This comment has been minimized.

@cjihrig

cjihrig Dec 3, 2016

Contributor

Can you wrap this callback in common.mustCall().

@cjihrig

cjihrig Dec 3, 2016

Contributor

Can you wrap this callback in common.mustCall().

Show outdated Hide outdated test/parallel/test-http-client-reject-unexpected-agent.js
() => http.request(Object.assign(baseOptions, {agent})),
(err) => {
return err.message === 'Agent option must be an instance of ' +
'http.Agent, undefined or false. Received type: ' +

This comment has been minimized.

@cjihrig

cjihrig Dec 3, 2016

Contributor

This line, and the following line, should be indented more.

@cjihrig

cjihrig Dec 3, 2016

Contributor

This line, and the following line, should be indented more.

Show outdated Hide outdated test/parallel/test-http-client-reject-unexpected-agent.js
);
});
const intervalId = setInterval(() => {

This comment has been minimized.

@cjihrig

cjihrig Dec 3, 2016

Contributor

Instead of using a timer (which tends to be flaky), and counting requests received, can you use common.mustCall() with it's second option to define how many times the server request handler is run.

@cjihrig

cjihrig Dec 3, 2016

Contributor

Instead of using a timer (which tends to be flaky), and counting requests received, can you use common.mustCall() with it's second option to define how many times the server request handler is run.

This comment has been minimized.

@cjihrig

cjihrig Dec 3, 2016

Contributor

That would make the process.on('exit', ...) handler unnecessary as well.

@cjihrig

cjihrig Dec 3, 2016

Contributor

That would make the process.on('exit', ...) handler unnecessary as well.

This comment has been minimized.

@brad-decker

brad-decker Dec 3, 2016

Contributor

@cjihrig - what is the correct way to .close() the server with this kind of test. Ideally we wouldn't close it just after request x in case for some awful reason x + 1 request would have erroneously occurred which would need to be caught?

I've been looking in some of the other http type tests but all seem to implement something different.

@brad-decker

brad-decker Dec 3, 2016

Contributor

@cjihrig - what is the correct way to .close() the server with this kind of test. Ideally we wouldn't close it just after request x in case for some awful reason x + 1 request would have erroneously occurred which would need to be caught?

I've been looking in some of the other http type tests but all seem to implement something different.

This comment has been minimized.

@brad-decker

brad-decker Dec 3, 2016

Contributor

Also 👍 on using the .mustCall function. I'll read through common.js in the future for test helpers.

@brad-decker

brad-decker Dec 3, 2016

Contributor

Also 👍 on using the .mustCall function. I'll read through common.js in the future for test helpers.

This comment has been minimized.

@cjihrig

cjihrig Dec 3, 2016

Contributor

I guess you might still need the counter to close the server. You might be able to get away with just closing the server after verifying that no exception was thrown, but that probably isn't a good idea. You could also use something like Promise.all() I suppose.

@cjihrig

cjihrig Dec 3, 2016

Contributor

I guess you might still need the counter to close the server. You might be able to get away with just closing the server after verifying that no exception was thrown, but that probably isn't a good idea. You could also use something like Promise.all() I suppose.

This comment has been minimized.

@brad-decker

brad-decker Dec 3, 2016

Contributor

Got a solution up that i think will work, if you could re-review i would appreciate it. Thanks!

@brad-decker

brad-decker Dec 3, 2016

Contributor

Got a solution up that i think will work, if you could re-review i would appreciate it. Thanks!

Show outdated Hide outdated lib/_http_client.js
@@ -14,7 +14,6 @@ const OutgoingMessage = require('_http_outgoing').OutgoingMessage;
const Agent = require('_http_agent');
const Buffer = require('buffer').Buffer;

This comment has been minimized.

@mscdex

mscdex Dec 3, 2016

Contributor

Unnecessary/unrelated change.

@mscdex

mscdex Dec 3, 2016

Contributor

Unnecessary/unrelated change.

This comment has been minimized.

@brad-decker

brad-decker Dec 3, 2016

Contributor

Addressed in a new commit and squashed

@brad-decker

brad-decker Dec 3, 2016

Contributor

Addressed in a new commit and squashed

Show outdated Hide outdated lib/_http_client.js
agent = defaultAgent;
}

This comment has been minimized.

@mscdex

mscdex Dec 3, 2016

Contributor

Unnecessary/unrelated change.

@mscdex

mscdex Dec 3, 2016

Contributor

Unnecessary/unrelated change.

This comment has been minimized.

@brad-decker

brad-decker Dec 3, 2016

Contributor

Addressed in a new commit and squashed

@brad-decker

brad-decker Dec 3, 2016

Contributor

Addressed in a new commit and squashed

Show outdated Hide outdated lib/_http_client.js
if (agent === false) {
agent = new defaultAgent.constructor();
} else if ((agent === null || agent === undefined) &&
typeof options.createConnection !== 'function') {
} else if (agent == null && typeof options.createConnection !== 'function') {
agent = defaultAgent;
}

This comment has been minimized.

@cjihrig

cjihrig Dec 4, 2016

Contributor

Instead of duplicating all the checks on line 34, couldn't you just add:

} else if (!(agent instanceof Agent.Agent)) {
  throw new TypeError(...);
}
@cjihrig

cjihrig Dec 4, 2016

Contributor

Instead of duplicating all the checks on line 34, couldn't you just add:

} else if (!(agent instanceof Agent.Agent)) {
  throw new TypeError(...);
}

This comment has been minimized.

@brad-decker

brad-decker Dec 4, 2016

Contributor

It's possible for agent to be null at this point, so I could do an else if here and check that it's not null and that it's not an instance of Agent.Agent.

I went this route as it verifies that we are dealing with appropriate types before this if statement and because the else if logic seemed slightly verbose.

I am okay with putting the extra checks on the else if statement though if it's preferred.

@brad-decker

brad-decker Dec 4, 2016

Contributor

It's possible for agent to be null at this point, so I could do an else if here and check that it's not null and that it's not an instance of Agent.Agent.

I went this route as it verifies that we are dealing with appropriate types before this if statement and because the else if logic seemed slightly verbose.

I am okay with putting the extra checks on the else if statement though if it's preferred.

This comment has been minimized.

@cjihrig

cjihrig Dec 5, 2016

Contributor

I think it's worth mixing into this logic in order to avoid duplicating all the checks.

@cjihrig

cjihrig Dec 5, 2016

Contributor

I think it's worth mixing into this logic in order to avoid duplicating all the checks.

This comment has been minimized.

@brad-decker

brad-decker Dec 5, 2016

Contributor

@cjihrig how about this logic structure:

if (agent === false) {
   // noop
} else if (agent == null) {
   if (typeof options.createConnection !== 'function') {
     // noop
   }
} else if (!(agent instanceof Agent.Agent)) {
  // noop throw error.
}

if agent is undefined or null it'll be cause by the second logic gate, and if createConnection is a function it will not do anything, and will skip the final check. The last else if is exactly as you defined earlier. Obviously the code snippet has implementation removed just for brevity.

@brad-decker

brad-decker Dec 5, 2016

Contributor

@cjihrig how about this logic structure:

if (agent === false) {
   // noop
} else if (agent == null) {
   if (typeof options.createConnection !== 'function') {
     // noop
   }
} else if (!(agent instanceof Agent.Agent)) {
  // noop throw error.
}

if agent is undefined or null it'll be cause by the second logic gate, and if createConnection is a function it will not do anything, and will skip the final check. The last else if is exactly as you defined earlier. Obviously the code snippet has implementation removed just for brevity.

This comment has been minimized.

@cjihrig

cjihrig Dec 5, 2016

Contributor

Seems like that would hit all of the cases.

@cjihrig

cjihrig Dec 5, 2016

Contributor

Seems like that would hit all of the cases.

This comment has been minimized.

@brad-decker

brad-decker Dec 6, 2016

Contributor

Just pushed that change, all tests passing locally 👍 thanks for the help @cjihrig

@brad-decker

brad-decker Dec 6, 2016

Contributor

Just pushed that change, all tests passing locally 👍 thanks for the help @cjihrig

Show outdated Hide outdated test/parallel/test-http-client-reject-unexpected-agent.js
closeServer = true;
}
request.on('response', common.mustCall(() => {
if (closeServer) server.close();

This comment has been minimized.

@cjihrig

cjihrig Dec 4, 2016

Contributor

Couldn't you just increment numberOfRequests here, and remove the closeServer variable. Also, can you move server.close() to a new line.

EDIT: You shouldn't really need to track the number of failures, since they should be captured by assert.throws().

@cjihrig

cjihrig Dec 4, 2016

Contributor

Couldn't you just increment numberOfRequests here, and remove the closeServer variable. Also, can you move server.close() to a new line.

EDIT: You shouldn't really need to track the number of failures, since they should be captured by assert.throws().

This comment has been minimized.

@brad-decker

brad-decker Dec 4, 2016

Contributor

numberOfRequests is counting the number of requests being initiated. Some of them are actually not actually hitting the server as they fail.

Definitely can move the server.close to new line

@brad-decker

brad-decker Dec 4, 2016

Contributor

numberOfRequests is counting the number of requests being initiated. Some of them are actually not actually hitting the server as they fail.

Definitely can move the server.close to new line

This comment has been minimized.

@brad-decker

brad-decker Dec 4, 2016

Contributor

@cjihrig addressed this in a new commit. Tracking now just the number of responses to close the server after receiving the expected number. Thanks

@brad-decker

brad-decker Dec 4, 2016

Contributor

@cjihrig addressed this in a new commit. Tracking now just the number of responses to close the server after receiving the expected number. Thanks

@jasnell jasnell added the semver-major label Dec 5, 2016

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Dec 5, 2016

Member

Marking semver-major because of the added throw

Member

jasnell commented Dec 5, 2016

Marking semver-major because of the added throw

@brad-decker

This comment has been minimized.

Show comment
Hide comment
@brad-decker

brad-decker Dec 6, 2016

Contributor

@bnoordhuis at your convenience would you mind taking a gander at this again?

Contributor

brad-decker commented Dec 6, 2016

@bnoordhuis at your convenience would you mind taking a gander at this again?

@cjihrig

cjihrig approved these changes Dec 6, 2016

Show outdated Hide outdated lib/_http_client.js
@@ -30,11 +30,18 @@ function ClientRequest(options, cb) {
var agent = options.agent;
var defaultAgent = options._defaultAgent || Agent.globalAgent;

This comment has been minimized.

@sam-github

sam-github Dec 6, 2016

Member

unrelated whitespace change

@sam-github

sam-github Dec 6, 2016

Member

unrelated whitespace change

Show outdated Hide outdated lib/_http_client.js
} else if ((agent === null || agent === undefined) &&
typeof options.createConnection !== 'function') {
agent = defaultAgent;
} else if (agent == null) {

This comment has been minimized.

@sam-github

sam-github Dec 6, 2016

Member

unrelated refactor. including purely stylistic changes in the same commit as a functional change makes PRs harder to review, and your commit message doesn't mention or justify these changes. These should be two commits, the addition of the else if (!(agent instanceof Agent.Agent)) { is the functional change, the other commit would be the style changes (though I would drop the addition of the random whitespace line, that's just code churn).

@sam-github

sam-github Dec 6, 2016

Member

unrelated refactor. including purely stylistic changes in the same commit as a functional change makes PRs harder to review, and your commit message doesn't mention or justify these changes. These should be two commits, the addition of the else if (!(agent instanceof Agent.Agent)) { is the functional change, the other commit would be the style changes (though I would drop the addition of the random whitespace line, that's just code churn).

This comment has been minimized.

@brad-decker

brad-decker Dec 6, 2016

Contributor

@sam-github i can actually not change this else if statement/block at all if i also check that agent is != null on my new else if (the functional change you mentioned). Other reviewers said that we should avoid duplicating the checks. What would be best in this case? I can split it into two commits as well just want to make sure i'm following best practices.

@brad-decker

brad-decker Dec 6, 2016

Contributor

@sam-github i can actually not change this else if statement/block at all if i also check that agent is != null on my new else if (the functional change you mentioned). Other reviewers said that we should avoid duplicating the checks. What would be best in this case? I can split it into two commits as well just want to make sure i'm following best practices.

This comment has been minimized.

@sam-github

sam-github Dec 6, 2016

Member

I misread the code, I see now, you pulled the check on createConnection out of the conditional, so that if it is a function, no check is made on the type of agent. Code like this is a bit hard to read, I suggest just adding a comment between line 39 and 40 below, making it clear that this case is explicitly being dropped through - that agent can be == null if there is a createConnection function, that would have caused me to not misread.

@sam-github

sam-github Dec 6, 2016

Member

I misread the code, I see now, you pulled the check on createConnection out of the conditional, so that if it is a function, no check is made on the type of agent. Code like this is a bit hard to read, I suggest just adding a comment between line 39 and 40 below, making it clear that this case is explicitly being dropped through - that agent can be == null if there is a createConnection function, that would have caused me to not misread.

This comment has been minimized.

@brad-decker

brad-decker Dec 7, 2016

Contributor

@sam-github at your convenience could you check the comment. Wasn't too sure on the wording so it seems a little verbose to me given i was struggling on explaining the case. I'm open to rewrite. Thanks again for your feedback.

@brad-decker

brad-decker Dec 7, 2016

Contributor

@sam-github at your convenience could you check the comment. Wasn't too sure on the wording so it seems a little verbose to me given i was struggling on explaining the case. I'm open to rewrite. Thanks again for your feedback.

const failingAgentOptions = [
true,
'agent',
{},

This comment has been minimized.

@sam-github

sam-github Dec 6, 2016

Member

would be good to inject a couple other js types here: Function, Number, Symbol

@sam-github

sam-github Dec 6, 2016

Member

would be good to inject a couple other js types here: Function, Number, Symbol

This comment has been minimized.

@brad-decker

brad-decker Dec 7, 2016

Contributor

added those types

@brad-decker

brad-decker Dec 7, 2016

Contributor

added those types

Show outdated Hide outdated test/parallel/test-http-client-reject-unexpected-agent.js
new http.Agent(),
];

This comment has been minimized.

@sam-github

sam-github Dec 6, 2016

Member

random whitespace - you have single line between global scope vars everywhere but here

@sam-github

sam-github Dec 6, 2016

Member

random whitespace - you have single line between global scope vars everywhere but here

Show outdated Hide outdated lib/_http_client.js
} else if ((agent === null || agent === undefined) &&
typeof options.createConnection !== 'function') {
agent = defaultAgent;
} else if (agent == null) {

This comment has been minimized.

@jasnell

jasnell Dec 7, 2016

Member

Please use (agent === null) (strict equals)

@jasnell

jasnell Dec 7, 2016

Member

Please use (agent === null) (strict equals)

This comment has been minimized.

@sam-github

sam-github Dec 7, 2016

Member

@jasnell it looks to me that it is required to be non-strict to match undefined and null

@sam-github

sam-github Dec 7, 2016

Member

@jasnell it looks to me that it is required to be non-strict to match undefined and null

This comment has been minimized.

@brad-decker

brad-decker Dec 7, 2016

Contributor

@sam-github @jasnell its non strict to catch both types but i did take the liberty of making that change whereas original version strictly checked for both null and undefined. So i'm flexible on implementation here.

@brad-decker

brad-decker Dec 7, 2016

Contributor

@sam-github @jasnell its non strict to catch both types but i did take the liberty of making that change whereas original version strictly checked for both null and undefined. So i'm flexible on implementation here.

Show outdated Hide outdated lib/_http_client.js
// explicitly pass through this statement as agent will not be used
// when createConnection is provided.
} else if (!(agent instanceof Agent.Agent)) {
throw new Error(

This comment has been minimized.

@jasnell

jasnell Dec 7, 2016

Member

Please use TypeError here

@jasnell

jasnell Dec 7, 2016

Member

Please use TypeError here

Show outdated Hide outdated test/parallel/test-http-client-reject-unexpected-agent.js
failingAgentOptions.forEach((agent) => {
assert.throws(
() => createRequest(agent),
(err) => {

This comment has been minimized.

@jasnell

jasnell Dec 7, 2016

Member

instead of a function here, just use a regexp... e.g.

assert.throws(() => createRequest(agent),
  /^TypeError: Agent option must be an instance of http.Agent/);
@jasnell

jasnell Dec 7, 2016

Member

instead of a function here, just use a regexp... e.g.

assert.throws(() => createRequest(agent),
  /^TypeError: Agent option must be an instance of http.Agent/);
Show outdated Hide outdated test/parallel/test-http-client-reject-unexpected-agent.js
acceptableAgentOptions.forEach((agent) => {
assert.doesNotThrow(
() => createRequest(agent),
Error,

This comment has been minimized.

@jasnell

jasnell Dec 7, 2016

Member

TypeError

@jasnell

jasnell Dec 7, 2016

Member

TypeError

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 12, 2016

Member

@brad-decker What I mean is that you can simply test assert.doesNotThrow(() => createRequest(agent)), there is no need to check for error types.

@bnoordhuis

bnoordhuis Dec 12, 2016

Member

@brad-decker What I mean is that you can simply test assert.doesNotThrow(() => createRequest(agent)), there is no need to check for error types.

@jasnell

Almost there... just a few bits remaining

Show outdated Hide outdated lib/_http_client.js
if (typeof options.createConnection !== 'function') {
agent = defaultAgent;
}
// Case when agent == null && typeof createConnection === 'function'

This comment has been minimized.

@sam-github

sam-github Dec 7, 2016

Member

This line just restates the code, I would drop it, and capitalize the "Explicitly" on the next sentence, so its clear that not handling this case is intentional.

@sam-github

sam-github Dec 7, 2016

Member

This line just restates the code, I would drop it, and capitalize the "Explicitly" on the next sentence, so its clear that not handling this case is intentional.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Dec 7, 2016

Member

Other than the one line of the comment that I would drop, LGTM

Member

sam-github commented Dec 7, 2016

Other than the one line of the comment that I would drop, LGTM

@brad-decker

This comment has been minimized.

Show comment
Hide comment
@brad-decker

brad-decker Dec 8, 2016

Contributor

@jasnell @sam-github - Your requests should now be addressed. Thanks!

Contributor

brad-decker commented Dec 8, 2016

@jasnell @sam-github - Your requests should now be addressed. Thanks!

@brad-decker

This comment has been minimized.

Show comment
Hide comment
@brad-decker

brad-decker Dec 10, 2016

Contributor

@jasnell @bnoordhuis sorry to bug you, just wondering if there is anything else I need to do to get this ready for CI. I know it might be a bit before it lands, so no rush. just curious.

Contributor

brad-decker commented Dec 10, 2016

@jasnell @bnoordhuis sorry to bug you, just wondering if there is anything else I need to do to get this ready for CI. I know it might be a bit before it lands, so no rush. just curious.

@bnoordhuis

LGTM modulo some final comments.

Show outdated Hide outdated test/parallel/test-http-client-reject-unexpected-agent.js
const baseOptions = {
method: 'GET',
port: undefined,
host: '127.0.0.1',

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 11, 2016

Member

Can you use common.localhostIPv4 here?

@bnoordhuis

bnoordhuis Dec 11, 2016

Member

Can you use common.localhostIPv4 here?

This comment has been minimized.

@brad-decker

brad-decker Dec 11, 2016

Contributor

I have this change in, waiting for discussion on your other requests prior to committing that up though. :D 👍

@brad-decker

brad-decker Dec 11, 2016

Contributor

I have this change in, waiting for discussion on your other requests prior to committing that up though. :D 👍

Show outdated Hide outdated test/parallel/test-http-client-reject-unexpected-agent.js
acceptableAgentOptions.forEach((agent) => {
assert.doesNotThrow(
() => createRequest(agent),
TypeError,

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 11, 2016

Member

I'd leave out the TypeError; any exception is bad.

@bnoordhuis

bnoordhuis Dec 11, 2016

Member

I'd leave out the TypeError; any exception is bad.

This comment has been minimized.

@brad-decker

brad-decker Dec 11, 2016

Contributor

The specific purpose of this test is that the acceptable list of agents are allowed, though. If it were to fail for some other reason wouldn't that be the purview of another test? @jasnell's change requests asked for this to be specified to TypeError (to be fair, they also asked for the Error thrown by incorrect agent types to also be classified appropriately as a TypeError)?

@brad-decker

brad-decker Dec 11, 2016

Contributor

The specific purpose of this test is that the acceptable list of agents are allowed, though. If it were to fail for some other reason wouldn't that be the purview of another test? @jasnell's change requests asked for this to be specified to TypeError (to be fair, they also asked for the Error thrown by incorrect agent types to also be classified appropriately as a TypeError)?

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 11, 2016

Member

It's better for a test to overreach and that we have to tighten it later on than that it's so specific that regressions can slip through.

@bnoordhuis

bnoordhuis Dec 11, 2016

Member

It's better for a test to overreach and that we have to tighten it later on than that it's so specific that regressions can slip through.

This comment has been minimized.

@brad-decker

brad-decker Dec 12, 2016

Contributor

Thanks! Changed this back to Error instead of TypeError.

@brad-decker

brad-decker Dec 12, 2016

Contributor

Thanks! Changed this back to Error instead of TypeError.

res.end('hello');
});
let numberOfResponses = 0;

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 11, 2016

Member

Can you add a process.on('exit', () => assert.strictEqual(numberOfResponses, acceptableAgentOptions.length))?

@bnoordhuis

bnoordhuis Dec 11, 2016

Member

Can you add a process.on('exit', () => assert.strictEqual(numberOfResponses, acceptableAgentOptions.length))?

This comment has been minimized.

@brad-decker

brad-decker Dec 11, 2016

Contributor

the numberOfResponses is really only used to close the server. The callback for the .on('response') listener is wrapped in common.mustCall() and that is called once per request, so if any fail the whole thing should fail. Is that acceptable or do we need to also count the responses on exit?

@brad-decker

brad-decker Dec 11, 2016

Contributor

the numberOfResponses is really only used to close the server. The callback for the .on('response') listener is wrapped in common.mustCall() and that is called once per request, so if any fail the whole thing should fail. Is that acceptable or do we need to also count the responses on exit?

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 11, 2016

Member

'Need' is too big a word but it's a cheap extra sanity check.

@bnoordhuis

bnoordhuis Dec 11, 2016

Member

'Need' is too big a word but it's a cheap extra sanity check.

This comment has been minimized.

@brad-decker

brad-decker Dec 12, 2016

Contributor

Understood. Added this check to the test file. Thanks!

@brad-decker

brad-decker Dec 12, 2016

Contributor

Understood. Added this check to the test file. Thanks!

@brad-decker

This comment has been minimized.

Show comment
Hide comment
@brad-decker

brad-decker Dec 12, 2016

Contributor

@bnoordhuis addressed your feedback. Thanks!

Contributor

brad-decker commented Dec 12, 2016

@bnoordhuis addressed your feedback. Thanks!

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Dec 12, 2016

Member

LGTM

Member

sam-github commented Dec 12, 2016

LGTM

@brad-decker

This comment has been minimized.

Show comment
Hide comment
@brad-decker

brad-decker Dec 13, 2016

Contributor

@bnoordhuis fixed the last bit of feedback you had, thanks again for reviewing and my apologies for the delay!

Contributor

brad-decker commented Dec 13, 2016

@bnoordhuis fixed the last bit of feedback you had, thanks again for reviewing and my apologies for the delay!

@brad-decker

This comment has been minimized.

Show comment
Hide comment
@brad-decker

brad-decker Dec 19, 2016

Contributor

@bnoordhuis - Can you point me in the direction of any resources that might help me figure out the CI testing process so i can address issues with the /arm and /linux tests? Would love to become more familiar with this process so i can be self sufficient here. As it stands i'm not quite sure what has failed.

Contributor

brad-decker commented Dec 19, 2016

@bnoordhuis - Can you point me in the direction of any resources that might help me figure out the CI testing process so i can address issues with the /arm and /linux tests? Would love to become more familiar with this process so i can be self sufficient here. As it stands i'm not quite sure what has failed.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Dec 19, 2016

Contributor

This is the only failure in the CI run - https://ci.nodejs.org/job/node-test-commit-linux/6776/nodes=ubuntu1610-x64/console (search for "not ok"). It doesn't appear to be related to the changes in this PR.

Contributor

cjihrig commented Dec 19, 2016

This is the only failure in the CI run - https://ci.nodejs.org/job/node-test-commit-linux/6776/nodes=ubuntu1610-x64/console (search for "not ok"). It doesn't appear to be related to the changes in this PR.

@brad-decker

This comment has been minimized.

Show comment
Hide comment
@brad-decker

brad-decker Dec 19, 2016

Contributor

Cool. Thanks @cjihrig - ill wait for feedback that specifies otherwise prior to delving into this.

Contributor

brad-decker commented Dec 19, 2016

Cool. Thanks @cjihrig - ill wait for feedback that specifies otherwise prior to delving into this.

@mscdex mscdex removed the lts-watch-v6.x label Dec 21, 2016

Show outdated Hide outdated lib/_http_client.js
} else if (!(agent instanceof Agent.Agent)) {
throw new TypeError(
'Agent option must be an instance of http.Agent, undefined or false. ' +
`Received type: ${typeof agent} instead`

This comment has been minimized.

@jasnell

jasnell Jan 11, 2017

Member

I'm really not that fond of the Received type: ... additional bits here.

@jasnell

jasnell Jan 11, 2017

Member

I'm really not that fond of the Received type: ... additional bits here.

This comment has been minimized.

@brad-decker

brad-decker Jan 14, 2017

Contributor

@jasnell - sorry for the delay. Addressed your feedback and removed the extra bits.

@brad-decker

brad-decker Jan 14, 2017

Contributor

@jasnell - sorry for the delay. Addressed your feedback and removed the extra bits.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@italoacasas

This comment has been minimized.

Show comment
Hide comment
@italoacasas

italoacasas Jan 17, 2017

Member

@brad-decker can you run make lint and fix the error?

Member

italoacasas commented Jan 17, 2017

@brad-decker can you run make lint and fix the error?

@brad-decker

This comment has been minimized.

Show comment
Hide comment
@brad-decker

brad-decker Jan 17, 2017

Contributor

@italoacasas i don't get any errors when running make lint. I also examined the output of the linter failure in CI and it seems to be a build error rather than a true lint error? I may be wrong i'm pretty new to this whole contributing to node thing.

Contributor

brad-decker commented Jan 17, 2017

@italoacasas i don't get any errors when running make lint. I also examined the output of the linter failure in CI and it seems to be a build error rather than a true lint error? I may be wrong i'm pretty new to this whole contributing to node thing.

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Jan 17, 2017

Member

@brad-decker I'm getting an error when I patch your PR onto master, could you rebase and see if you get an error?

➜  curl -L https://github.com/nodejs/node/pull/10053.patch | git am --whitespace=fix"
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   139    0   139    0     0    341      0 --:--:-- --:--:-- --:--:--   342
100  3369    0  3369    0     0   3770      0 --:--:-- --:--:-- --:--:-- 24237
Applying: http: throw an error for unexpected agent values
➜  node git:(master) make lint
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules \
	  benchmark lib test tools

/Users/gib/wrk/com/node/test/parallel/test-http-client-reject-unexpected-agent.js
  52:8  error  Expected indentation of 6 spaces but found 7  indent

✖ 1 problem (1 error, 0 warnings)

make: *** [jslint] Error 1
Member

gibfahn commented Jan 17, 2017

@brad-decker I'm getting an error when I patch your PR onto master, could you rebase and see if you get an error?

➜  curl -L https://github.com/nodejs/node/pull/10053.patch | git am --whitespace=fix"
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   139    0   139    0     0    341      0 --:--:-- --:--:-- --:--:--   342
100  3369    0  3369    0     0   3770      0 --:--:-- --:--:-- --:--:-- 24237
Applying: http: throw an error for unexpected agent values
➜  node git:(master) make lint
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules \
	  benchmark lib test tools

/Users/gib/wrk/com/node/test/parallel/test-http-client-reject-unexpected-agent.js
  52:8  error  Expected indentation of 6 spaces but found 7  indent

✖ 1 problem (1 error, 0 warnings)

make: *** [jslint] Error 1
Show outdated Hide outdated test/parallel/test-http-client-reject-unexpected-agent.js
failingAgentOptions.forEach((agent) => {
assert.throws(
() => createRequest(agent),
/^TypeError: Agent option must be an instance of http.Agent/,

This comment has been minimized.

@gibfahn

gibfahn Jan 17, 2017

Member

Looks like one too many spaces on this line

@gibfahn

gibfahn Jan 17, 2017

Member

Looks like one too many spaces on this line

http: throw an error for unexpected agent values
As per #9069 unexpected things can happen
when supplying an unexpected value to agent.
Beings as the docs clearly state the expected
values, this throws an error on an unexpected
value.

Signed-off-by: brad-decker <bhdecker84@gmail.com>
@brad-decker

This comment has been minimized.

Show comment
Hide comment
@brad-decker

brad-decker Jan 17, 2017

Contributor

@italoacasas @gibfahn I see what i did. i rebased on origin and not upstream :P got the lint error after rebasing. Sorry about that. fixed now

Contributor

brad-decker commented Jan 17, 2017

@italoacasas @gibfahn I see what i did. i rebased on origin and not upstream :P got the lint error after rebasing. Sorry about that. fixed now

@italoacasas

This comment has been minimized.

Show comment
Hide comment
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Jan 18, 2017

Member

CI is green (ignore the test/arm reported failure)

Member

gibfahn commented Jan 18, 2017

CI is green (ignore the test/arm reported failure)

@italoacasas

This comment has been minimized.

Show comment
Hide comment
@italoacasas
Member

italoacasas commented Jan 18, 2017

Landed fc7025c

italoacasas added a commit that referenced this pull request Jan 18, 2017

http: throw an error for unexpected agent values
As per #9069
unexpected things can happen when supplying an
unexpected value to agent. Beings as the docs
clearly state the expected values, this throws
an error on an unexpected value.

PR-URL: #10053
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@brad-decker

This comment has been minimized.

Show comment
Hide comment
@brad-decker

brad-decker Jan 18, 2017

Contributor

Thanks everyone!

Contributor

brad-decker commented Jan 18, 2017

Thanks everyone!

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017

http: throw an error for unexpected agent values
As per nodejs#9069
unexpected things can happen when supplying an
unexpected value to agent. Beings as the docs
clearly state the expected values, this throws
an error on an unexpected value.

PR-URL: nodejs#10053
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

@lance lance referenced this pull request Jan 19, 2017

Closed

http core module issue #9069

}
// Explicitly pass through this statement as agent will not be used
// when createConnection is provided.
} else if (!(agent instanceof Agent.Agent)) {

This comment has been minimized.

@fengmk2

fengmk2 Feb 26, 2017

Member

How about userland Agent? Those Agent Class maybe not inherits from Agent.Agent and now will be all fails to use them. e.g.: TunnelingAgent https://github.com/request/tunnel-agent/blob/master/index.js#L47

@fengmk2

fengmk2 Feb 26, 2017

Member

How about userland Agent? Those Agent Class maybe not inherits from Agent.Agent and now will be all fails to use them. e.g.: TunnelingAgent https://github.com/request/tunnel-agent/blob/master/index.js#L47

@fengmk2 fengmk2 referenced this pull request Feb 26, 2017

Closed

http: should support userland Agent #11567

2 of 4 tasks complete

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment