http core module issue

on Node.js 6.7

I got this while using require('http').request()

[accounts.api.test.js]  stack: TypeError: self.agent.addRequest is not a function
[accounts.api.test.js]     at new ClientRequest (_http_client.js:158:16)
[accounts.api.test.js]     at Object.exports.request (http.js:31:10)
[accounts.api.test.js]     at Object.makeHTTPRequest [as httpHelper] (/Users/t_millal/WebstormProjects/autodesk/crucible-poc-discovery/test/helpers/http-helper.js:22:20)
[accounts.api.test.js]     at TestSuiteBase.it.cb.t (/Users/t_millal/WebstormProjects/autodesk/crucible-poc-discovery/test/test-src/accounts.api.test.js:27:17)
[accounts.api.test.js]     at /Users/t_millal/WebstormProjects/oresoftware/suman/lib/test-suite-helpers/handle-test.js:191:37
[accounts.api.test.js]     at _combinedTickCallback (internal/process/next_tick.js:67:7)
[accounts.api.test.js]     at process._tickDomainCallback (internal/process/next_tick.js:122:9)

probably shouldn't be happening no matter what the input data is, but here it is:


[accounts.api.test.js]  => request d => 
[accounts.api.test.js]  { host: 'localhost',
[accounts.api.test.js]   port: 3001,
[accounts.api.test.js]   path: '/api/Accounts',
[accounts.api.test.js]   method: 'GET',
[accounts.api.test.js]   agent: true,   <<< seems to be culprit
[accounts.api.test.js]   headers: { 'Content-Type': 'application/json' } }

curious as to what might be happening thanks, setting agent to false, will prevent this error being thrown...

The http documentation shows valid values for the agent property. true is not one of them.

well shoot, maybe http should throw a better error when you pass an unacceptable value as an option..right?

If it accepts false, it seems like it should also accept true for consistency (which would be equivalent to not providing the parameter).

right, this is a little weird tbh

instead of false, it should accept null or undefined perhaps

I've added the 'good first contribution' label but if no one picks it up within the next few weeks I'll go ahead and close it out as 'working as documented.'

Ok so more definitely, here are the current docs on this:

https://nodejs.org/api/http.html#http_http_request_options_callback

it says:

agent: Controls Agent behavior. When an Agent is used request will default to Connection: keep-alive. Possible values:
undefined (default): use http.globalAgent for this host and port.
Agent object: explicitly use the passed in Agent.
false: opts out of connection pooling with an Agent, defaults request to Connection: close.

Let's talk about the above, but as an aside, let's say we did allow agent:true, couldn't the HTTP core module deliver some "default" HTTP agent? or does it really have to be a user defined agent?

seems like you could do this:

=> http.request options =>

 { host: 'localhost',
  port: 3001,
 path: '/api/Accounts',
method: 'GET',
agent: 'default',   // if true or 'default', then HTTP core module will procure an agent with default options
 headers: { 'Content-Type': 'application/json' } }

so my thinking is here are the options:

value for "agent" option =>	undefined	false	true	(new) HTTP.Agent	'default'
result	same as current docs	same as current docs	same as 'default'	same as current docs	would create a new HTTP.Agent with default options

So I think passing null should throw an error (if it doesn't already). And either true should throw an error (it doesn't now) or true/'default' should create some default new HTTP.Agent. Right now the situation is not acceptable IMO.

https://nodejs.org/api/http.html#http_new_agent_options

@the1mills

@the1mills, thanks for putting your thoughts into this. I have a few comments on your suggested options:

So I think passing null should throw an error

I'm not sure I agree with this. I don't think we should distinguish between null and undefined as inputs, as that will lead to confusion. This would also be a breaking change, which I think is unnecessary in this case.

'default'

I'm not sure I understand the point of having an explicit option for 'default'. If someone explicitly wants to use the default behavior, they can simply omit the property and have the default behavior take effect.

In my opinion, the best course of action would be to simply allow agent: true to mean "yes, use an agent". This would nicely contrast with the existing agent: false option (which means "no agent"), and it also happens to be the same as the default behavior, which is to use an agent if agent is undefined.

I think it just makes things more weird. Having undefined (which is falsy) and false mean different things is odd, its not what someone would expect without reading the docs.

I would expect null to be falsy, and be the same as undefined or false... but that can't work: falsy like undefined, or falsy like false? Hm... I don't think there is anything we can do here that would be expected, the best thing would be to throw clear errors on any use of an invalid argument value, and expect people to read the docs.

I think agent: false semantically means "don't use an agent". agent: true semantically means "use an agent". Explicitly listing agent: undefined does seem sort of weird, and I'm not sure what it should mean, but a more common case would be omitting the agent property entirely, which triggers the default behavior.

We could use hasOwnProperty to detect whether agent: undefined is explicitly passed, but I think changing the behavior based on hasOwnProperty will be confusing. Similarly, I think treating null any differently than undefined will be confusing.

I don't think the API that I proposed above is overly confusing; if we allow agent: true, then the option is basically a boolean value that defaults to true. (This ignores the ability to pass a different agent instead of a boolean, but I think that can be considered separately.)

@the1mills

Well, just my opinion here, but I still think this is rearranging the deck chairs on the titanic. It isn't going to make sense when undefined and false are defined to mean the opposite of each other.

The sensible api would be:

no agent property: do not use an agent
agent: an agent object, use the agent
agent: http.defaultAgent, use the default agent
agent: true,false: not agents, throw error
agent: undefined: like no agent property, do not use an agent
agent: null: like no agent property, do not use an agent

but that's very semver major :-)

And back to the main point. @the1mills got thrown off because of a terrible error message when they used the API in a way that was plainly documented to be wrong - just a decent error message would have avoided all this, and caused the OP to check the docs.

Fixing that is a good first contribution!

right I was saying agent:undefined is the same as
!options.hasOwnProperty('agent'),

should result in same thing

as for agent:true, it needs to be one of three things:

~agent:false, (would not make sense at all)
~agent:undefined,
or some 3rd thing

my vote is that agent:true procures some default agent of some variety
On Oct 24, 2016 1:58 PM, "Teddy Katz" notifications@github.com wrote:

I think agent: false semantically means "don't use an agent". agent: true
semantically means "use an agent". Explicitly listing agent: undefined
does seem sort of weird, and I'm not sure what it should mean, but a more
common case would be omitting the agent property entirely, which triggers
the default behavior.

We could use hasOwnProperty to detect whether agent: undefined is
explicitly passed, but I think changing the behavior based on
hasOwnProperty will be confusing. Similarly, I think treating null any
differently than undefined will be confusing.

I don't think this API is overly confusing; if we allow agent: true, then
the option is basically a boolean value that defaults to true. (This
ignores the ability to pass a different agent instead of a boolean, but I
think that can be considered separately.)

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#9069 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AKn56It73KYeA5X9C7iItolz3K8PQRqaks5q3RvxgaJpZM4KVWCl
.

http: throw an error for unexpected agent values

@sam-github

I think the semver-major recommendation from @sam-github makes sense. But yeah... it's semver-major. In any case, it seems that this issue has been resolved with #10053 so I'm going to close this.

Please feel free to reopen if I am mistaken.

Navigation Menu

http core module issue #9069

1 remaining item

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

http core module issue #9069

Description

Activity

mscdex commented on Oct 13, 2016

ORESoftware commented on Oct 13, 2016

not-an-aardvark commented on Oct 15, 2016

ORESoftware commented on Oct 15, 2016

ORESoftware commented on Oct 15, 2016

bnoordhuis commented on Oct 21, 2016

the1mills commented on Oct 24, 2016

not-an-aardvark commented on Oct 24, 2016

sam-github commented on Oct 24, 2016

not-an-aardvark commented on Oct 24, 2016

sam-github commented on Oct 25, 2016

ORESoftware commented on Oct 25, 2016

1 remaining item

lance commented on Jan 19, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions