Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1903263002:
DNS: Expose stale results through HostResolverImpl. (Closed)
|
Created:
4 years, 8 months ago by Julia Tuttle Modified:
4 years, 6 months ago Reviewers:
Randy Smith (Not in Mondays) CC:
cbentzel+watch_chromium.org, chromium-reviews, mmenke Base URL:
https://chromium.googlesource.com/chromium/src.git@dns_stale1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDNS: Expose stale results through HostResolverImpl.
Once the HostCache exposes stale results, plumb them through the
HostResolverImpl for callers who want them.
BUG=605138
Committed: https://crrev.com/9fb7aebfa1f3b6576ab0d26333936489b300ac16
Cr-Commit-Position: refs/heads/master@{#398109}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 : rebase #Patch Set 7 : rebase #
Total comments: 4
Patch Set 8 : Clarify interface. #Patch Set 9 : rebase #
Total comments: 14
Patch Set 10 : Make requested changes, plus some others. #Patch Set 11 : rebase #
Total comments: 9
Patch Set 12 : Make a few more changes #Patch Set 13 : rebase #
Total comments: 6
Patch Set 14 : rebase, update comments, tighten DCHECKs, remove ResolveStale() #Patch Set 15 : Add unittest for ResolveStaleFromCache. #Patch Set 16 : rebase #Patch Set 17 : Remove ResolveStaleFromCache from HostResolver but not Impl #
Dependent Patchsets: Messages
Total messages: 23 (8 generated)
Description was changed from ========== WIP: DNS: Expose stale results through HostResolver(Impl). Once the HostCache exposes stale results, plumb them through the HostResolver for callers who want them. BUG=605138 ========== to ========== WIP: DNS: Expose stale results through HostResolver(Impl). Once the HostCache exposes stale results, plumb them through the HostResolver for callers who want them. TODO: Need unittests, but didn't see a super obvious way to add them. BUG=605138 ==========
juliatuttle@chromium.org changed reviewers: + rdsmith@chromium.org
PTAL, rdsmith. Unittests are still pending; feel free to ignore until they're ready if you like.
My primary concerns here are about the interface (there isn't a lot of code in this CL) so I'm only going to make interface comments until we resolve any issues around the format and name of StaleEntryInfo in the CL this depends on. https://codereview.chromium.org/1903263002/diff/120001/net/dns/host_resolver.h File net/dns/host_resolver.h (right): https://codereview.chromium.org/1903263002/diff/120001/net/dns/host_resolver.... net/dns/host_resolver.h:168: // (Defaults to calling |ResolveFromCache()| if not overridden.) This implies that |stale_info| is not filled in if there isn't a stale response, in which case how can we reliably tell that there isn't a stale response? Requiring the caller to fill in stale_info to say "not stale" before calling doesn't seem reasonable. Maybe have this contract always fill in stale_info so there's a reliable way to check? https://codereview.chromium.org/1903263002/diff/120001/net/dns/host_resolver.... net/dns/host_resolver.h:187: // |Resolve()| and sets |*stale_error| to ERR_DNS_CACHE_MISS. Confirming: All cases that start a network request return ERR_IO_PENDING?
PTAL. https://codereview.chromium.org/1903263002/diff/120001/net/dns/host_resolver.h File net/dns/host_resolver.h (right): https://codereview.chromium.org/1903263002/diff/120001/net/dns/host_resolver.... net/dns/host_resolver.h:168: // (Defaults to calling |ResolveFromCache()| if not overridden.) On 2016/04/28 at 16:29:55, Randy Smith - Not in Fridays wrote: > This implies that |stale_info| is not filled in if there isn't a stale response, in which case how can we reliably tell that there isn't a stale response? Requiring the caller to fill in stale_info to say "not stale" before calling doesn't seem reasonable. Maybe have this contract always fill in stale_info so there's a reliable way to check? Oh, it's supposed to (and does) always fill in stale_info *if* there's a response (which you can tell from the return code). I'll document that. https://codereview.chromium.org/1903263002/diff/120001/net/dns/host_resolver.... net/dns/host_resolver.h:187: // |Resolve()| and sets |*stale_error| to ERR_DNS_CACHE_MISS. On 2016/04/28 at 16:29:55, Randy Smith - Not in Fridays wrote: > Confirming: All cases that start a network request return ERR_IO_PENDING? Yup. Should be clearer in the new version of the comment.
Tests? https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.cc File net/dns/host_resolver.cc (right): https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... net/dns/host_resolver.cc:134: *stale_addresses = *addresses; I wince a bit at setting *stale_addresses in the ERR_DNS_CACHE_MISS case, since nothing was returned in that case. https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... net/dns/host_resolver.cc:135: addresses->clear(); So both this and the ResolveStaleFromCache() call above mean that the function may alter |*addresses| when it's not actually returning fresh information synchronously. I could imagine this being surprising behavior to the caller, so I'd suggest avoiding it even though it makes the implementation more complicated (I neither think the caller should rely on *addresses not being modified nor that the implementation should modify it, if synchronous fresh results aren't being returned). https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.h File net/dns/host_resolver.h (right): https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... net/dns/host_resolver.h:168: // is. (Defaults to calling |ResolveFromCache()| if not overridden.) I'd personally vote for (i.e.: suggestion) leaving out the parenthetical comment and just saying "... can return a stale result if the implementation supports that behavior." The "if not overridden" feels like implementation information to me. https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... net/dns/host_resolver.h:181: // Otherwise, starts a network request, returns ERR_IO_PENDING, and: nit: This is implicitly relying on the the comment above Resolve() about the handling of |out_req|. My preference would be for each of these comments to be standalone (or only rely on top level comments) as opposed to having inter-comment dependencies; could you duplicate the description of the handling of out_req from Resolve()? (By the way, thanks for the doc update--this is complex, but clear.) https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... net/dns/host_resolver.h:188: int ResolveStale(const RequestInfo& info, In not marking this virtual, you're specifically indicating that implementations can't override it. Is that a conscious choice? What are the design considerations behind it? https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver_... File net/dns/host_resolver_impl.h (right): https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver_... net/dns/host_resolver_impl.h:174: // HOSTS and is not localhost. nit: Update with comment about extra argument? (ETA: This is especially important as it looks to me as if the behavior of this function is different depending on whether stale_info is null.) https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver_... net/dns/host_resolver_impl.h:192: // if it is a positive entry. Ditto comment about extra argument?
PTAL. Neither of us noticed that ResolveStaleFromCache wasn't setting stale_info in the literal/hosts/localhost cases, so those results might be deemed stale depending on what was randomly in stale_info before. I fixed it by specifically setting it to a known-not-stale set of constants in those cases. https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.cc File net/dns/host_resolver.cc (right): https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... net/dns/host_resolver.cc:134: *stale_addresses = *addresses; On 2016/05/04 19:46:59, Randy Smith - Not in Fridays wrote: > I wince a bit at setting *stale_addresses in the ERR_DNS_CACHE_MISS case, since > nothing was returned in that case. Fixed. https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... net/dns/host_resolver.cc:135: addresses->clear(); On 2016/05/04 19:46:59, Randy Smith - Not in Fridays wrote: > So both this and the ResolveStaleFromCache() call above mean that the function > may alter |*addresses| when it's not actually returning fresh information > synchronously. I could imagine this being surprising behavior to the caller, so > I'd suggest avoiding it even though it makes the implementation more complicated > (I neither think the caller should rely on *addresses not being modified nor > that the implementation should modify it, if synchronous fresh results aren't > being returned). Fixed. https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.h File net/dns/host_resolver.h (right): https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... net/dns/host_resolver.h:168: // is. (Defaults to calling |ResolveFromCache()| if not overridden.) On 2016/05/04 19:46:59, Randy Smith - Not in Fridays wrote: > I'd personally vote for (i.e.: suggestion) leaving out the parenthetical comment > and just saying "... can return a stale result if the implementation supports > that behavior." The "if not overridden" feels like implementation information > to me. Done. https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... net/dns/host_resolver.h:181: // Otherwise, starts a network request, returns ERR_IO_PENDING, and: On 2016/05/04 19:46:59, Randy Smith - Not in Fridays wrote: > nit: This is implicitly relying on the the comment above Resolve() about the > handling of |out_req|. My preference would be for each of these comments to be > standalone (or only rely on top level comments) as opposed to having > inter-comment dependencies; could you duplicate the description of the handling > of out_req from Resolve()? > > (By the way, thanks for the doc update--this is complex, but clear.) Done. https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... net/dns/host_resolver.h:188: int ResolveStale(const RequestInfo& info, On 2016/05/04 19:46:59, Randy Smith - Not in Fridays wrote: > In not marking this virtual, you're specifically indicating that implementations > can't override it. Is that a conscious choice? What are the design > considerations behind it? It is a conscious choice -- I see ResolveStale as a specific combination of calls to ResolveStaleFromCache and Resolve. I don't see a purpose in letting a subclass override it. If you do, I'll happily make it virtual, or we can just leave it this way until someone finds a reason to change it. https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver_... File net/dns/host_resolver_impl.h (right): https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver_... net/dns/host_resolver_impl.h:174: // HOSTS and is not localhost. On 2016/05/04 19:46:59, Randy Smith - Not in Fridays wrote: > nit: Update with comment about extra argument? > > (ETA: This is especially important as it looks to me as if the behavior of this > function is different depending on whether stale_info is null.) Done. https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver_... net/dns/host_resolver_impl.h:192: // if it is a positive entry. On 2016/05/04 19:46:59, Randy Smith - Not in Fridays wrote: > Ditto comment about extra argument? Done.
On 2016/05/05 15:42:54, Julia Tuttle wrote: > PTAL. > > Neither of us noticed that ResolveStaleFromCache wasn't setting stale_info in > the literal/hosts/localhost cases, so those results might be deemed stale > depending on what was randomly in stale_info before. > > I fixed it by specifically setting it to a known-not-stale set of constants in > those cases. > > https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.cc > File net/dns/host_resolver.cc (right): > > https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... > net/dns/host_resolver.cc:134: *stale_addresses = *addresses; > On 2016/05/04 19:46:59, Randy Smith - Not in Fridays wrote: > > I wince a bit at setting *stale_addresses in the ERR_DNS_CACHE_MISS case, > since > > nothing was returned in that case. > > Fixed. > > https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... > net/dns/host_resolver.cc:135: addresses->clear(); > On 2016/05/04 19:46:59, Randy Smith - Not in Fridays wrote: > > So both this and the ResolveStaleFromCache() call above mean that the function > > may alter |*addresses| when it's not actually returning fresh information > > synchronously. I could imagine this being surprising behavior to the caller, > so > > I'd suggest avoiding it even though it makes the implementation more > complicated > > (I neither think the caller should rely on *addresses not being modified nor > > that the implementation should modify it, if synchronous fresh results aren't > > being returned). > > Fixed. > > https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.h > File net/dns/host_resolver.h (right): > > https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... > net/dns/host_resolver.h:168: // is. (Defaults to calling |ResolveFromCache()| if > not overridden.) > On 2016/05/04 19:46:59, Randy Smith - Not in Fridays wrote: > > I'd personally vote for (i.e.: suggestion) leaving out the parenthetical > comment > > and just saying "... can return a stale result if the implementation supports > > that behavior." The "if not overridden" feels like implementation information > > to me. > > Done. > > https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... > net/dns/host_resolver.h:181: // Otherwise, starts a network request, returns > ERR_IO_PENDING, and: > On 2016/05/04 19:46:59, Randy Smith - Not in Fridays wrote: > > nit: This is implicitly relying on the the comment above Resolve() about the > > handling of |out_req|. My preference would be for each of these comments to > be > > standalone (or only rely on top level comments) as opposed to having > > inter-comment dependencies; could you duplicate the description of the > handling > > of out_req from Resolve()? > > > > (By the way, thanks for the doc update--this is complex, but clear.) > > Done. > > https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver.... > net/dns/host_resolver.h:188: int ResolveStale(const RequestInfo& info, > On 2016/05/04 19:46:59, Randy Smith - Not in Fridays wrote: > > In not marking this virtual, you're specifically indicating that > implementations > > can't override it. Is that a conscious choice? What are the design > > considerations behind it? > > It is a conscious choice -- I see ResolveStale as a specific combination of > calls to ResolveStaleFromCache and Resolve. I don't see a purpose in letting a > subclass override it. > > If you do, I'll happily make it virtual, or we can just leave it this way until > someone finds a reason to change it. > > https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver_... > File net/dns/host_resolver_impl.h (right): > > https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver_... > net/dns/host_resolver_impl.h:174: // HOSTS and is not localhost. > On 2016/05/04 19:46:59, Randy Smith - Not in Fridays wrote: > > nit: Update with comment about extra argument? > > > > (ETA: This is especially important as it looks to me as if the behavior of > this > > function is different depending on whether stale_info is null.) > > Done. > > https://codereview.chromium.org/1903263002/diff/160001/net/dns/host_resolver_... > net/dns/host_resolver_impl.h:192: // if it is a positive entry. > On 2016/05/04 19:46:59, Randy Smith - Not in Fridays wrote: > > Ditto comment about extra argument? > > Done. I'll call out that I don't think you responded to my top level "Tests?" comment on my last review.
Sorry, now I see you responded to my tests question, but in the CL description :-}. I'm not sure I know enough to help you add them, but it might be something amenable to brainstorming--let's chat about it on Monday. https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver.cc File net/dns/host_resolver.cc (right): https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver.... net/dns/host_resolver.cc:112: stale_info->stale_hits = 0u; The interface spec says that |*stale_info| is only filled in if some value is returned. https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver.h File net/dns/host_resolver.h (right): https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver.... net/dns/host_resolver.h:180: // returned to indicate how stale (or not) it is. Based on host_resolver_impl.cc, it's valid to call this function with a null stale_info. If that's the case, I think it should be documented here (I take the above description as implying that it isn't, and I don't think it's a good idea to allow for different implementations that do different things in this space; it'll cause a null pointer dereference at some point down the line.) https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver.... net/dns/host_resolver.h:186: // Uses |ResolveStaleFromCache| and |Resolve| to get fresh data (either from Just putting in a reminder note (to us both) that we may want to modify this interface based on the results of the conversation on the other CL around inheritance versus adding methods for giving access to stale info. (One possible way to deal with this section of that larger issue is just nuking this interface; as I understand it, it's just syntactic sugar wrapping around Resolve and ResolveStaleFromCache, and I'm not sure it would be that unreasonable to force callers that are interested in the issue to implement it themselves. And it's a pretty complex interface and behavior for just being a helper routine.) (Going further down the path of deciding I don't like this interface: If we're going to implement a helper function for some use, I'd really like to see a use case or two to make sure we're getting the best semantics for the helper function. Without that, keeping the interface minimal seems higher priority.) https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver_... File net/dns/host_resolver_impl.h (right): https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver_... net/dns/host_resolver_impl.h:180: // an entry, fresh or stale, was returned). (I'll remark in passing that I'm twitchy about the nullity of an output parameter control a function's behavior and I'd rather have the control done some other way (maybe a separate function?), but given this is a well documented, private, helper function that doesn't really even rise to the level of suggestion; I'm good with you making your own decision. But I wanted to express my discomfort :-}.)
PTAL, rdsmith. https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver.cc File net/dns/host_resolver.cc (right): https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver.... net/dns/host_resolver.cc:112: stale_info->stale_hits = 0u; On 2016/05/07 01:36:31, Randy Smith - Not in Fridays wrote: > The interface spec says that |*stale_info| is only filled in if some value is > returned. Oh, good call. Fixed to set those only if ResolveFromCache doesn't return ERR_DNS_CACHE_MISS. https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver.h File net/dns/host_resolver.h (right): https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver.... net/dns/host_resolver.h:180: // returned to indicate how stale (or not) it is. On 2016/05/07 01:36:31, Randy Smith - Not in Fridays wrote: > Based on host_resolver_impl.cc, it's valid to call this function with a null > stale_info. If that's the case, I think it should be documented here (I take > the above description as implying that it isn't, and I don't think it's a good > idea to allow for different implementations that do different things in this > space; it'll cause a null pointer dereference at some point down the line.) Yeah, it shouldn't be. I've added a DCHECK. https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver.... net/dns/host_resolver.h:186: // Uses |ResolveStaleFromCache| and |Resolve| to get fresh data (either from On 2016/05/07 01:36:31, Randy Smith - Not in Fridays wrote: > Just putting in a reminder note (to us both) that we may want to modify this > interface based on the results of the conversation on the other CL around > inheritance versus adding methods for giving access to stale info. > > (One possible way to deal with this section of that larger issue is just nuking > this interface; as I understand it, it's just syntactic sugar wrapping around > Resolve and ResolveStaleFromCache, and I'm not sure it would be that > unreasonable to force callers that are interested in the issue to implement it > themselves. And it's a pretty complex interface and behavior for just being a > helper routine.) > > (Going further down the path of deciding I don't like this interface: If we're > going to implement a helper function for some use, I'd really like to see a use > case or two to make sure we're getting the best semantics for the helper > function. Without that, keeping the interface minimal seems higher priority.) If you want, I can move it into the StaleHostResolver, and if and when we do this kind of thing in multiple places, we can factor it back out in a sensible way? https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver_... File net/dns/host_resolver_impl.h (right): https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver_... net/dns/host_resolver_impl.h:180: // an entry, fresh or stale, was returned). On 2016/05/07 01:36:31, Randy Smith - Not in Fridays wrote: > (I'll remark in passing that I'm twitchy about the nullity of an output > parameter control a function's behavior and I'd rather have the control done > some other way (maybe a separate function?), but given this is a well > documented, private, helper function that doesn't really even rise to the level > of suggestion; I'm good with you making your own decision. But I wanted to > express my discomfort :-}.) Done.
Next round of comments. I think we're pretty close (modulo tests). https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver.h File net/dns/host_resolver.h (right): https://codereview.chromium.org/1903263002/diff/200001/net/dns/host_resolver.... net/dns/host_resolver.h:186: // Uses |ResolveStaleFromCache| and |Resolve| to get fresh data (either from On 2016/05/11 20:19:47, Julia Tuttle wrote: > On 2016/05/07 01:36:31, Randy Smith - Not in Fridays wrote: > > Just putting in a reminder note (to us both) that we may want to modify this > > interface based on the results of the conversation on the other CL around > > inheritance versus adding methods for giving access to stale info. > > > > (One possible way to deal with this section of that larger issue is just > nuking > > this interface; as I understand it, it's just syntactic sugar wrapping around > > Resolve and ResolveStaleFromCache, and I'm not sure it would be that > > unreasonable to force callers that are interested in the issue to implement it > > themselves. And it's a pretty complex interface and behavior for just being a > > helper routine.) > > > > (Going further down the path of deciding I don't like this interface: If we're > > going to implement a helper function for some use, I'd really like to see a > use > > case or two to make sure we're getting the best semantics for the helper > > function. Without that, keeping the interface minimal seems higher priority.) > > If you want, I can move it into the StaleHostResolver, and if and when we do > this kind of thing in multiple places, we can factor it back out in a sensible > way? I think I'd really prefer that. Thanks. https://codereview.chromium.org/1903263002/diff/240001/net/dns/host_resolver.cc File net/dns/host_resolver.cc (right): https://codereview.chromium.org/1903263002/diff/240001/net/dns/host_resolver.... net/dns/host_resolver.cc:133: *stale_error = ERR_UNEXPECTED; Why set |*stale_error| if it isn't stale? Isn't that against the interface spec? https://codereview.chromium.org/1903263002/diff/240001/net/dns/host_resolver_... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1903263002/diff/240001/net/dns/host_resolver_... net/dns/host_resolver_impl.cc:2022: DCHECK(!allow_stale || stale_info); Should document in declaration comment that if allow_stale is true, stale_info must be non-null. https://codereview.chromium.org/1903263002/diff/240001/net/dns/host_resolver_... File net/dns/host_resolver_impl.h (right): https://codereview.chromium.org/1903263002/diff/240001/net/dns/host_resolver_... net/dns/host_resolver_impl.h:180: // an entry, fresh or stale, was returned). I think this comment needs to be updated? |allow_stale| now controls if stale entries may be returned (yay!). Ditto below.
PTAL, rdsmith. (Still need to write tests.) https://codereview.chromium.org/1903263002/diff/240001/net/dns/host_resolver.cc File net/dns/host_resolver.cc (right): https://codereview.chromium.org/1903263002/diff/240001/net/dns/host_resolver.... net/dns/host_resolver.cc:133: *stale_error = ERR_UNEXPECTED; On 2016/05/17 20:39:35, Randy Smith - Not in Fridays wrote: > Why set |*stale_error| if it isn't stale? Isn't that against the interface > spec? It's mostly defensive -- if someone accidentally gets the logic wrong and acts on |*stale_error|, I want them to return a known-bogus error instead of potentially returning a random error or erroneous results based on whatever was left there. I'm moving this into StaleHostResolver, so it will no longer be against a spec promise I'm making to other callers. https://codereview.chromium.org/1903263002/diff/240001/net/dns/host_resolver_... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1903263002/diff/240001/net/dns/host_resolver_... net/dns/host_resolver_impl.cc:2022: DCHECK(!allow_stale || stale_info); On 2016/05/17 20:39:35, Randy Smith - Not in Fridays wrote: > Should document in declaration comment that if allow_stale is true, stale_info > must be non-null. Done. https://codereview.chromium.org/1903263002/diff/240001/net/dns/host_resolver_... File net/dns/host_resolver_impl.h (right): https://codereview.chromium.org/1903263002/diff/240001/net/dns/host_resolver_... net/dns/host_resolver_impl.h:180: // an entry, fresh or stale, was returned). On 2016/05/17 20:39:35, Randy Smith - Not in Fridays wrote: > I think this comment needs to be updated? |allow_stale| now controls if stale > entries may be returned (yay!). Ditto below. Done.
Description was changed from ========== WIP: DNS: Expose stale results through HostResolver(Impl). Once the HostCache exposes stale results, plumb them through the HostResolver for callers who want them. TODO: Need unittests, but didn't see a super obvious way to add them. BUG=605138 ========== to ========== DNS: Expose stale results through HostResolver(Impl). Once the HostCache exposes stale results, plumb them through the HostResolver for callers who want them. BUG=605138 ==========
I added a unittest for HostResolverImpl::ResolveStaleFromCache. PTAL, rdsmith.
Ok. I'm good with this as written, so LGTM. But :-J. My personal favorite resolution to the issues currently being discussed on https://codereview.chromium.org/1898873006 will be to pull ResolveStaleFromCache (and possibly ResolveFromCache) out of the HostResolver interface and make them only visible on the HostResolverImpl interface (because of the conceptual clash between exposing stale results via a reimplementation of the interface vs. exposing them via explicit methods). So you might want to wait for the resolution of that CL to land this one. Or you might be good with maybe yanking those interfaces back out depending on the results of the discussion on that CL (and them putting them back in when you hoist the logic in StaleHostResolver up into TransportConnectJob). I figure I'll leave your comfort with those possible Do-si-do steps up to you.
Description was changed from ========== DNS: Expose stale results through HostResolver(Impl). Once the HostCache exposes stale results, plumb them through the HostResolver for callers who want them. BUG=605138 ========== to ========== DNS: Expose stale results through HostResolverImpl. Once the HostCache exposes stale results, plumb them through the HostResolverImpl for callers who want them. BUG=605138 ==========
The CQ bit was checked by juliatuttle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1903263002/#ps320001 (title: "Remove ResolveStaleFromCache from HostResolver but not Impl")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1903263002/320001
Message was sent while issue was closed.
Description was changed from ========== DNS: Expose stale results through HostResolverImpl. Once the HostCache exposes stale results, plumb them through the HostResolverImpl for callers who want them. BUG=605138 ========== to ========== DNS: Expose stale results through HostResolverImpl. Once the HostCache exposes stale results, plumb them through the HostResolverImpl for callers who want them. BUG=605138 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== DNS: Expose stale results through HostResolverImpl. Once the HostCache exposes stale results, plumb them through the HostResolverImpl for callers who want them. BUG=605138 ========== to ========== DNS: Expose stale results through HostResolverImpl. Once the HostCache exposes stale results, plumb them through the HostResolverImpl for callers who want them. BUG=605138 Committed: https://crrev.com/9fb7aebfa1f3b6576ab0d26333936489b300ac16 Cr-Commit-Position: refs/heads/master@{#398109} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/9fb7aebfa1f3b6576ab0d26333936489b300ac16 Cr-Commit-Position: refs/heads/master@{#398109} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
