Fix the `agent: null` http agent regression #7189

Closed
wants to merge 5 commits into
from

Projects

None yet

4 participants

@TooTallNate

See the individual commit messages for more details. I'd be fine with squashing this as well if you guys think that would be better. /cc @tjfontaine @indutny @bnoordhuis

See #7012.

TooTallNate added some commits Feb 25, 2014
@TooTallNate TooTallNate test: add failing http `agent: null` test
See #7012.
472079e
@TooTallNate TooTallNate http, https: don't depend on `globalAgent`
For the `request()` and `get()` functions. I could never
really understand why these two functions go through agent
first... Especially since the user could be passing `agent: false`
or a different Agent instance completely, in which `globalAgent`
will be completely bypassed.

Moved the relevant logic from `Agent#request()` into the
`ClientRequest` constructor.

Incidentally, this commit fixes #7012 (which was the original
intent of this commit).
e5b9126
@TooTallNate TooTallNate test: update "http-*" tests to only use public API
Don't invoke the `agent.requst()` or `agent.get()` functions
directly. Instead, use the public API and pass the agent
instance in as the `agent` option.
c1707f4
@TooTallNate TooTallNate http: remove the circular dependency
Between `ClientRequest` and `Agent`. The circular require was doing
weird things at load time, like making the `globalAgent` property
be `undefined` from within the context of the "_http_client"
module.

Removing the circular dependency completely fixes this.
4426079
@Nodejs-Jenkins

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

  • Nathan Rajlich

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@TooTallNate

@bnoordhuis I'm also planning on fixing the createConnection option for you, which appears to be completely broken on master branch... That'll be a separate PR though most likely.

@bnoordhuis

That looks like commit 7124387. Did that get lost somewhere along the way?

I suppose it should be broken out into a separate commit because it's not really related to the agent fix.

Member

Wait, never mind - you're just moving it around.

@bnoordhuis
Member

Typo in commit log: s/requst/request/

@bnoordhuis
Member

LGTM. Cheers, Nathan.

@TooTallNate

/cc @isaacs TJ said that you had some reservations about agent.request()/agent.get(), which I'm removing here to eliminate the problematic cyclic dependency between Agent and ClientRequest. Did you have any comments about this?

@indutny
Member
indutny commented Feb 26, 2014

The test seems to be hanging in latest v0.10, mind having a look?

@indutny
Member
indutny commented Feb 26, 2014

Ah, it was with your createConnection patch.

@indutny
Member

hm? should you pass method in options?

Member

nvm.

@indutny
Member
indutny commented Feb 26, 2014

LGTM, I like it!

@TooTallNate TooTallNate added a commit to TooTallNate/node that referenced this pull request Feb 26, 2014
@TooTallNate TooTallNate test: add `agent: null` http client request test
This is just the test portion from #7012 / #7189,
but targetted for the v0.10 branch.
47abdd9
@TooTallNate

@indutny re: test failing on v0.10. There was an issue with the test I found. See TooTallNate@47abdd9 for a better test (I'll update this PR with the updated test as well).

@TooTallNate TooTallNate test: consume the response in `agent: null` test
Otherwise the process would hang open on v0.10
(though on master the old test would pass successfully).

Also adding counter checks for both the server's
"request" event and the client's "response" event.
cec684a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment