code review 148970043: net/http: make Transport.CloseIdleConnections also clos...

net/http: make Transport.CloseIdleConnections also close pending dials

See comment 4 of https://code.google.com/p/go/issues/detail?id=8483#c4:

"So if a user creates a http.Client, issues a bunch of
requests and then wants to shutdown it and all opened connections;
what is she intended to do? The report suggests that just waiting for
all pending requests and calling CloseIdleConnections won't do, as
there can be new racing connections. Obviously she can't do what
you've done in the test, as it uses the unexported function.  If this
happens periodically, it can lead to serious resource leaks (the
transport is also preserved alive).  Am I missing something?"

This CL tracks the user's intention to close all idle
connections (CloseIdleConnections sets it true; and making a
new request sets it false). If a pending dial finishes and
nobody wants it, before it's retained for a future caller, the
"wantIdle" bool is checked and it's closed if the user has
called CloseIdleConnections without a later call to make a new
request.

Fixes Issue 8483

[bradfitz, 2014-09-24 17:32:59]

Hello golang-codereviews@googlegroups.com (cc: adg@golang.org,
dvyukov@google.com, rsc@golang.org),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dvyukov, 2014-09-24 18:36:39]

Can we revert the test changes and test hooks now?
The idea is that the original code in the test must magically work now.

[bradfitz, 2014-09-24 18:45:49]

No.

This CL only says that the pending connections will eventually be shut down
(when they finish connecting) so that there is no resource leak. But it
doesn't make CloseIdleConnections block waiting for them to be closed.  I
thought that was too much of a behavior change, and also too much code for
such an unimportant edge case.  But the test cares about ordering, so the
hooks remain.

On Wed, Sep 24, 2014 at 11:36 AM, <dvyukov@google.com> wrote:

> Can we revert the test changes and test hooks now?
> The idea is that the original code in the test must magically work now.
>
>
> https://codereview.appspot.com/148970043/
>

[adg, 2014-09-25 00:02:47]

LGTM

[bradfitz, 2014-09-25 00:05:24]

I'll wait to add a test for this first, and then ask for another review.


On Wed, Sep 24, 2014 at 5:02 PM, <adg@golang.org> wrote:

> LGTM
>
> https://codereview.appspot.com/148970043/
>

[bradfitz, 2014-09-29 18:28:29]

PTAL Andrew (or anybody) now with tests. Nothing too interesting.


On Wed, Sep 24, 2014 at 5:05 PM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:

> I'll wait to add a test for this first, and then ask for another review.
>
>
> On Wed, Sep 24, 2014 at 5:02 PM, <adg@golang.org> wrote:
>
>> LGTM
>>
>> https://codereview.appspot.com/148970043/
>>
>
>

[adg, 2014-09-30 00:56:06]

LGTM

[bradfitz, 2014-09-30 01:16:04]

*** Submitted as https://code.google.com/p/go/source/detail?r=6573961dc33e ***

net/http: make Transport.CloseIdleConnections also close pending dials

See comment 4 of https://code.google.com/p/go/issues/detail?id=8483#c4:

"So if a user creates a http.Client, issues a bunch of
requests and then wants to shutdown it and all opened connections;
what is she intended to do? The report suggests that just waiting for
all pending requests and calling CloseIdleConnections won't do, as
there can be new racing connections. Obviously she can't do what
you've done in the test, as it uses the unexported function.  If this
happens periodically, it can lead to serious resource leaks (the
transport is also preserved alive).  Am I missing something?"

This CL tracks the user's intention to close all idle
connections (CloseIdleConnections sets it true; and making a
new request sets it false). If a pending dial finishes and
nobody wants it, before it's retained for a future caller, the
"wantIdle" bool is checked and it's closed if the user has
called CloseIdleConnections without a later call to make a new
request.

Fixes Issue 8483

LGTM=adg
R=golang-codereviews, dvyukov, adg
CC=golang-codereviews, rsc
https://codereview.appspot.com/148970043