Only mark a proxy as bad if we have confirmation that another proxy succeeded for the same request.

Only mark a proxy as bad if we have confirmation that another proxy succeeded for the same request.

BUG=87336
TEST=net_unittests --gtest_filter=ProxyServiceTest.ProxyFallback:HttpStreamFactoryTest.JobNotifiesProxy


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98643

[asanka, 2011-08-17 17:16:52]

This patch defers the addition of a proxy to the list of bad proxies until we
have confirmation that another proxy choice (could be DIRECT) had better
success.  Since the primary purpose of the bad proxy cache currently is to
prioritize the proxy list, this effectively delays an addition to the cache
until we have evidence that doing so leads to an improvement.

There's also a unit test for Jingle, that I'm going to put into another CL
(assuming this CL is good) because of a few changes I had to make to improve the
testability of proxy_resolving_client_socket.

[eroman, 2011-08-19 02:12:59]

I'm trying to review this right now, but am getting some 500s from
codereview.
I'll keep trying, but if I can't get that resolved by the EOD I may not have
comments for you until tommorow!

Cheers.

On Wed, Aug 17, 2011 at 10:16 AM, <asanka@chromium.org> wrote:

> Reviewers: eroman,
>
> Message:
> This patch defers the addition of a proxy to the list of bad proxies until
> we
> have confirmation that another proxy choice (could be DIRECT) had better
> success.  Since the primary purpose of the bad proxy cache currently is to
> prioritize the proxy list, this effectively delays an addition to the cache
> until we have evidence that doing so leads to an improvement.
>
> There's also a unit test for Jingle, that I'm going to put into another CL
> (assuming this CL is good) because of a few changes I had to make to
> improve the
> testability of proxy_resolving_client_socket.
>
>
> Description:
> Only mark a proxy as bad if we have confirmation that another proxy
> succeeded
> for the same request.
>
> BUG=87336
> TEST=net_unittests
> --gtest_filter=**ProxyServiceTest.**ProxyFallback:**HttpStreamFactoryTest.
> **JobNotifiesProxy
>
>
> Please review this at
http://codereview.chromium.**org/7532011/<http://codereview.chromium.org/7532...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M jingle/notifier/base/proxy_**resolving_client_socket.h
>  M jingle/notifier/base/proxy_**resolving_client_socket.cc
>  M net/base/net_log_event_type_**list.h
>  M net/http/http_stream_factory_**impl_job.cc
>  M net/http/http_stream_factory_**impl_unittest.cc
>  M net/proxy/proxy_info.h
>  M net/proxy/proxy_info.cc
>  M net/proxy/proxy_list.h
>  M net/proxy/proxy_list.cc
>  M net/proxy/proxy_service.h
>  M net/proxy/proxy_service.cc
>  M net/proxy/proxy_service_**unittest.cc
>
>
>

[eroman, 2011-08-19 18:30:39]

Rietveld is working for me today, will do this review ASAP!

[eroman, 2011-08-19 21:25:32]

LGTM!

Thanks for adding the extra logging too.

http://codereview.chromium.org/7532011/diff/11001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

http://codereview.chromium.org/7532011/diff/11001/net/proxy/proxy_list.cc#new...
net/proxy/proxy_list.cc:123: // TODO(eroman): It would be good if instead of
removing failed proxies
My thinking in this comment was to do away with ProxyRetryInfoMap (since timing
isn't really relevant anymore), and just have ProxyList be a vector of proxy
choices (augmented by whether it was attempted and failed) along with a pointer
to the current location.

No need to change anything for this CL though, just thinking out loud.

http://codereview.chromium.org/7532011/diff/11001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/7532011/diff/11001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:272: iter != retry_info.end(); ++iter)
nit: Please use curly braces for the body.

http://codereview.chromium.org/7532011/diff/11001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/7532011/diff/11001/net/proxy/proxy_service.h#n...
net/proxy/proxy_service.h:103: void ReportSuccess(const ProxyInfo& proxy_info);
We will need to be very careful to call this in all the right places. I wander
if there is any way to make this more future-proof (I can't think of anything
though).

[asanka, 2011-08-19 22:09:37]

Thanks!

http://codereview.chromium.org/7532011/diff/11001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/7532011/diff/11001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:272: iter != retry_info.end(); ++iter)
On 2011/08/19 21:25:32, eroman wrote:
> nit: Please use curly braces for the body.

Done.

[commit-bot: I haz the power, 2011-08-29 16:40:01]

Change committed as 98643