Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(12)

Issue 118061: Use res_ninit for thread safety and a timer for reloading resolv.conf on Linux. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 12 months ago by Craig Schlenter
Modified:
6 years ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Use res_ninit for thread safety and a timer for reloading resolv.conf on Linux. We will only reload resolv.conf once per second per thread when DNS lookups fail. This should match the behaviour of mozilla. [retry of r17530, passes Valgrind now] BUG=12740

Patch Set 1 #

Patch Set 2 : restart timer when expired #

Patch Set 3 : use singleton and thread_local_storage #

Total comments: 4

Patch Set 4 : style fixes #

Total comments: 1

Patch Set 5 : add some prose #

Patch Set 6 : call res_nclose as per Lei Zhang #

Unified diffs Side-by-side diffs Delta from patch set Stats (+76 lines, -2 lines) Patch
M net/base/host_resolver.cc View 1 2 3 4 5 3 chunks +76 lines, -2 lines 0 comments Download
Trybot results:
Commit queue not available (can’t edit this change).

Messages

Total messages: 19 (0 generated)
Evan Martin
+dns expert
7 years, 12 months ago (2009-05-31 17:22:47 UTC) #1
Evan Martin
Since this data is stored per-thread, I think it would make more sense to use ...
7 years, 12 months ago (2009-05-31 17:25:57 UTC) #2
Craig Schlenter
On 2009/05/31 17:25:57, Evan Martin wrote: > Since this data is stored per-thread, I think ...
7 years, 12 months ago (2009-05-31 18:05:44 UTC) #3
Paweł Hajdan Jr.
res_ninit piece looks ok to me (I had to read more about it). Few nits: ...
7 years, 12 months ago (2009-06-01 11:54:54 UTC) #4
Craig Schlenter
Thanks for the review comments! I have not forgotten about this and will try to ...
7 years, 12 months ago (2009-06-03 04:30:52 UTC) #5
Craig Schlenter
I have uploaded a new patch that uses singleton and thread_local_storage. I haven't created a ...
7 years, 12 months ago (2009-06-03 17:49:04 UTC) #6
Evan Martin
The structure of this looks good to me, I have only style nits. http://codereview.chromium.org/118061/diff/7/1005 File ...
7 years, 12 months ago (2009-06-03 18:00:43 UTC) #7
Craig Schlenter
On 2009/06/03 18:00:43, Evan Martin wrote: > The structure of this looks good to me, ...
7 years, 12 months ago (2009-06-03 18:18:17 UTC) #8
Evan Martin
this LGTM, just one more comment request http://codereview.chromium.org/118061/diff/1007/11 File net/base/host_resolver.cc (right): http://codereview.chromium.org/118061/diff/1007/11#newcode49 Line 49: #if ...
7 years, 12 months ago (2009-06-03 18:25:32 UTC) #9
Craig Schlenter
> http://codereview.chromium.org/118061/diff/1007/11#newcode49 > Line 49: #if defined(OS_LINUX) > Would you mind expanding this comment to ...
7 years, 12 months ago (2009-06-03 18:54:27 UTC) #10
Evan Martin
LGTM, will commit
7 years, 12 months ago (2009-06-03 20:02:00 UTC) #11
Lei Zhang
Hey Craig, the valgrind bot says we're leaking memory now. Can you take a look? ...
7 years, 12 months ago (2009-06-03 23:53:01 UTC) #12
Craig Schlenter
On 2009/06/03 23:53:01, Lei Zhang wrote: > Hey Craig, the valgrind bot says we're leaking ...
7 years, 12 months ago (2009-06-04 03:41:19 UTC) #13
Craig Schlenter
On 2009/06/04 03:41:19, Craig Schlenter wrote: > On 2009/06/03 23:53:01, Lei Zhang wrote: > > ...
7 years, 12 months ago (2009-06-04 05:26:04 UTC) #14
willchan no longer on Chromium
On 2009/06/04 05:26:04, Craig Schlenter wrote: > On 2009/06/04 03:41:19, Craig Schlenter wrote: > > ...
7 years, 12 months ago (2009-06-04 05:30:55 UTC) #15
Craig Schlenter
> I see there is an undocumented (well it's not in the man page) res_close ...
7 years, 12 months ago (2009-06-04 05:37:10 UTC) #16
Lei Zhang
On 2009/06/04 05:37:10, Craig Schlenter wrote: > > I see there is an undocumented (well ...
7 years, 12 months ago (2009-06-04 06:51:13 UTC) #17
Craig Schlenter
On 2009/06/04 06:51:13, Lei Zhang wrote: > Does this work? (I'm not a dns expert ...
7 years, 12 months ago (2009-06-04 07:35:17 UTC) #18
Craig Schlenter
7 years, 12 months ago (2009-06-04 13:51:34 UTC) #19
On 2009/06/04 07:35:17, Craig Schlenter wrote:
> On 2009/06/04 06:51:13, Lei Zhang wrote:
> > Does this work? (I'm not a dns expert and I could be wrong)
> > 
> >   if (err && dns_timer->Expired()) {
> >     res_nclose(&_res);
> >     if (!res_ninit(&_res))
> >       err = getaddrinfo(host.c_str(), port.c_str(), &hints, out);
> >   }
> > 
> > Sleep is good. Oh look at the time! Zzzzz.
> 
> Thanks! I had thought of doing this (and I tried it now and it appears to
work)
> but calling close before init is potentially bad if init hasn't been called
> first I think.[snip]

I think the reason this is working is because the first call to any of the
resolver functions will end up initialising things through an internal glibc
function called __res_maybe_init ... since we have just done a failed dns lookup
before we call res_nclose, it should be safe to call res_nclose unconditionally.

Patch updated. Passes valgrind on my machine.

Thank you!
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06