Allow for an existing bad proxy to be given a new retry delay.

Allow for an existing bad proxy to be given a new retry delay.

Before this CL, any proxy that was previously marked as bad could not be
assigned a different retry delay, and could only be marked as bad for
the previous retry delay.

This CL changes the proxy retry map update logic to replace the existing
bad proxy retry info if the new retry time is later than the existing
retry time.

BUG=500738
Committed: https://crrev.com/08141d98705bc3a7ad0e53e64408adbf065eb554
Cr-Commit-Position: refs/heads/master@{#336451}

[sclittle, 2015-06-16 22:27:24]

sclittle@chromium.org changed reviewers:
+ mmenke@chromium.org

[sclittle, 2015-06-16 22:27:24]

mmenke: *

eroman CC'd since he's out on vacation.

Thanks in advance!

[mmenke, 2015-06-17 19:37:55]

On 2015/06/16 22:27:24, sclittle wrote:
> mmenke: *
> 
> eroman CC'd since he's out on vacation.
> 
> Thanks in advance!

Hope to get to this tomorrow, but may not until Friday - I want to fully
understand the effects of overwriting all 4 fields in this case before I can
sign off on this.

[mmenke, 2015-06-18 22:25:06]

https://codereview.chromium.org/1191823002/diff/20001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (left):

https://codereview.chromium.org/1191823002/diff/20001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:194: // double the retry time. Bug 997660.
Wow...This is a bug ID from before Chrome was open sourced.  Been around a
while.  :)

https://codereview.chromium.org/1191823002/diff/20001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/1191823002/diff/20001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:197: retry_info.try_while_bad = try_while_bad;
Seems like changing this could have unexpected side effects.

We have two fundamentally different types of proxies marked as bad, and not sure
we should just decide between the two based on which ones is bad until the date
furthest in the future.

[sclittle, 2015-06-19 01:45:10]

https://codereview.chromium.org/1191823002/diff/20001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/1191823002/diff/20001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:197: retry_info.try_while_bad = try_while_bad;
On 2015/06/18 22:25:06, mmenke wrote:
> Seems like changing this could have unexpected side effects.
> 
> We have two fundamentally different types of proxies marked as bad, and not
sure
> we should just decide between the two based on which ones is bad until the
date
> furthest in the future.

I see your point. Although I do think that this logic makes more sense than the
old logic, it could potentially cause a proxy to be retried that was marked as
try_while_bad=false before that retry delay expires, if that entry was
overwritten by a try_while_bad=true entry.

One way to solve this would be to make the proxy retry list into a multimap that
can support multiple entries for a proxy, and then just have the proxy
resolution logic obey the most restrictive non-expired retry info entry. That
would require quite a bit of changes throughout this codebase, and be a bit more
complicated though.

Another, simpler option would be to leave try_while_bad alone, and only update
current_delay, bad_until, etc., which would closely match the current
implementation. However, it would seem strange if marking an existing bad proxy
as bad would not respect the new try_while_bad value.

WDYT?

[mmenke, 2015-06-19 15:15:56]

https://codereview.chromium.org/1191823002/diff/20001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/1191823002/diff/20001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:197: retry_info.try_while_bad = try_while_bad;
On 2015/06/19 01:45:10, sclittle wrote:
> On 2015/06/18 22:25:06, mmenke wrote:
> > Seems like changing this could have unexpected side effects.
> > 
> > We have two fundamentally different types of proxies marked as bad, and not
> sure
> > we should just decide between the two based on which ones is bad until the
> date
> > furthest in the future.
> 
> I see your point. Although I do think that this logic makes more sense than
the
> old logic, it could potentially cause a proxy to be retried that was marked as
> try_while_bad=false before that retry delay expires, if that entry was
> overwritten by a try_while_bad=true entry.
> 
> One way to solve this would be to make the proxy retry list into a multimap
that
> can support multiple entries for a proxy, and then just have the proxy
> resolution logic obey the most restrictive non-expired retry info entry. That
> would require quite a bit of changes throughout this codebase, and be a bit
more
> complicated though.
> 
> Another, simpler option would be to leave try_while_bad alone, and only update
> current_delay, bad_until, etc., which would closely match the current
> implementation. However, it would seem strange if marking an existing bad
proxy
> as bad would not respect the new try_while_bad value.
> 
> WDYT?

Or could overwrite the old try_while_bad value if the old one was true (Or,
equivalently, if the new one is false).

Or could use the current logic if the try_while_bad values are the same, but if
they're different, prefer the one that's false.

I'm sufficiently concerned about this that I'd really rather have someone who
better understood the proxy retry logic weighed in.  Mind waiting for eroman to
get back?  He's back next week, I believe.  Don't think we have any other proxy
experts on the net/ team, unfortunately.

[sclittle, 2015-06-19 19:55:36]

sclittle@chromium.org changed reviewers:
+ eroman@chromium.org

[sclittle, 2015-06-19 19:55:36]

https://codereview.chromium.org/1191823002/diff/20001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/1191823002/diff/20001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:197: retry_info.try_while_bad = try_while_bad;
On 2015/06/19 15:15:56, mmenke wrote:
> On 2015/06/19 01:45:10, sclittle wrote:
> > On 2015/06/18 22:25:06, mmenke wrote:
> > > Seems like changing this could have unexpected side effects.
> > > 
> > > We have two fundamentally different types of proxies marked as bad, and
not
> > sure
> > > we should just decide between the two based on which ones is bad until the
> > date
> > > furthest in the future.
> > 
> > I see your point. Although I do think that this logic makes more sense than
> the
> > old logic, it could potentially cause a proxy to be retried that was marked
as
> > try_while_bad=false before that retry delay expires, if that entry was
> > overwritten by a try_while_bad=true entry.
> > 
> > One way to solve this would be to make the proxy retry list into a multimap
> that
> > can support multiple entries for a proxy, and then just have the proxy
> > resolution logic obey the most restrictive non-expired retry info entry.
That
> > would require quite a bit of changes throughout this codebase, and be a bit
> more
> > complicated though.
> > 
> > Another, simpler option would be to leave try_while_bad alone, and only
update
> > current_delay, bad_until, etc., which would closely match the current
> > implementation. However, it would seem strange if marking an existing bad
> proxy
> > as bad would not respect the new try_while_bad value.
> > 
> > WDYT?
> 
> Or could overwrite the old try_while_bad value if the old one was true (Or,
> equivalently, if the new one is false).
> 
> Or could use the current logic if the try_while_bad values are the same, but
if
> they're different, prefer the one that's false.
> 
> I'm sufficiently concerned about this that I'd really rather have someone who
> better understood the proxy retry logic weighed in.  Mind waiting for eroman
to
> get back?  He's back next week, I believe.  Don't think we have any other
proxy
> experts on the net/ team, unfortunately.

Sure, let's wait for eroman then. I've added him as a reviewer.

[eroman, 2015-06-25 16:55:25]

sure will take a look once i beat back more of my backlog

[eroman, 2015-06-26 14:43:58]

LGTM.

For future reference asanka@ has hacked on the bad proxy retry logic and would
be a suitable alternate reviewer in case I am gone.

[sclittle, 2015-06-26 19:45:22]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2015-06-26 19:45:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1191823002/20001

[commit-bot: I haz the power, 2015-06-26 20:51:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-26 20:52:15]

Patchset 2 (id:??) landed as
https://crrev.com/08141d98705bc3a7ad0e53e64408adbf065eb554
Cr-Commit-Position: refs/heads/master@{#336451}