Use FileWatcher to watch system resolver config, on systems that require...

Use FileWatcher to watch system resolver config, on systems that require
and support it (effectively Linux).

[eroman, 2011-01-04 23:15:27]

Adam/Evan, are you the right people to review this? Who is up to speed on the
dns reload timer stuff?

[Evan Martin, 2011-01-04 23:17:41]

I'll defer to Adam on this one.  Craig also helped with the original patches in
this area.

[willchan no longer on Chromium, 2011-01-04 23:50:40]

Drive-by comments.

Thanks for working on this change!  Can you motivate the change a bit more?  Is
there a reason you wish to use FileWatcher?  Are you changing the platform
defines to make this code only run on Linux systems?  There are a lot of
stylistic things that are incorrect in this change, probably due to your
unfamiliarity with Chromium style.  Before delving into those, I think it'd be
good to make sure that this change is desirable.  Can you update the changelist
description to explain why you're doing what you're doing?  Is switching to
FileWatcher solving an existing issue?

http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc
File net/base/dns_reload_timer.cc (right):

http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc#ne...
net/base/dns_reload_timer.cc:18: #include
"chrome/browser/file_path_watcher/file_path_watcher.h"
net is a lower level module than chrome.  chrome depends on net.  Not vice
versa.  So this dependency is unacceptable.

[rojer9, 2011-01-05 00:07:12]

On 2011/01/04 23:50:40, willchan wrote:
> Drive-by comments.
> 
> Thanks for working on this change!  Can you motivate the change a bit more?

this is my proposed solution for http://crbug.com/67734
the gist of it: resolv.conf needs to be re-read and current solution -
re-reading on failure - doesn't work in some network configurations.
watching file for changes and reloading resolv.conf if it has changed *before*
attempting resolution solves that problem and is generally a better sound
approach. though in this CL i'm not getting rid of the old one, my expectation
is that it's not going to matter anymore.

> Is there a reason you wish to use FileWatcher?

i wanted to use the inotify(7) interface and FileWatcher already provides a
handy wrapper around it (except for the dependency, which needs to be
addressed).

> Are you changing the platform defines to make this code only run on Linux
systems?

WATCH_RESOLV_CONF is just a more readable shortcut for the expression that was
being used. yes, as it stands now all of resolv.conf logic is only built on
linux and possibly freebsd: windows and os x don't need it and openbsd does not
support thread-safe reload of resolver configuration, so it's disabled there.
this is all in the original comments and i'm not changing anything substantial
here, just replaced a hairy expression with more readable one.

> There are a lot of
> stylistic things that are incorrect in this change, probably due to your
> unfamiliarity with Chromium style.  Before delving into those, I think it'd be
> good to make sure that this change is desirable.  Can you update the
changelist
> description to explain why you're doing what you're doing?  Is switching to
> FileWatcher solving an existing issue?

it does. i think the discussion on the bug should clarify what is being
addressed, why and how.

> 
> http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc
> File net/base/dns_reload_timer.cc (right):
> 
>
http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc#ne...
> net/base/dns_reload_timer.cc:18: #include
> "chrome/browser/file_path_watcher/file_path_watcher.h"
> net is a lower level module than chrome.  chrome depends on net.  Not vice
> versa.  So this dependency is unacceptable.

i agree, so i surmised when i was adding it :)
it would be a pity if inotify had to be re-written just because of the
dependency problem. maybe filewatcher could be pulled up?
alternatively, this code could be moved down to chrome/browser, but there needs
to be some way of communicating with resolver code in net/base.

[Craig, 2011-01-05 00:58:24]

On 2011/01/04 23:50:40, willchan wrote:
> Drive-by comments.
> 
> Thanks for working on this change!  Can you motivate the change a bit more? 
Is
> there a reason you wish to use FileWatcher?  Are you changing the platform
> defines to make this code only run on Linux systems?  There are a lot of
> stylistic things that are incorrect in this change, probably due to your
> unfamiliarity with Chromium style.  Before delving into those, I think it'd be
> good to make sure that this change is desirable.  Can you update the
changelist
> description to explain why you're doing what you're doing?  Is switching to
> FileWatcher solving an existing issue?
> 
> http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc
> File net/base/dns_reload_timer.cc (right):
> 
>
http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc#ne...
> net/base/dns_reload_timer.cc:18: #include
> "chrome/browser/file_path_watcher/file_path_watcher.h"
> net is a lower level module than chrome.  chrome depends on net.  Not vice
> versa.  So this dependency is unacceptable.

FileWatcher used to be part of base but it moved at some point. When I'm near
git/svn again I'll try to see why. Perhaps it can move back to base or
alternately this code may need to use inotify directly. There are also
non-notify options to solve the underlying issue such as stat as mentioned in
the bug but that's "expensive" to do on each lookup  but it will be much less
code/complex than inotify.

[willchan no longer on Chromium, 2011-01-05 01:01:07]

On Tue, Jan 4, 2011 at 4:58 PM, <craig.schlenter@chromium.org> wrote:

> On 2011/01/04 23:50:40, willchan wrote:
>
>> Drive-by comments.
>>
>
>  Thanks for working on this change!  Can you motivate the change a bit
>> more?
>>
> Is
>
>> there a reason you wish to use FileWatcher?  Are you changing the platform
>> defines to make this code only run on Linux systems?  There are a lot of
>>
>> stylistic things that are incorrect in this change, probably due to your
>> unfamiliarity with Chromium style.  Before delving into those, I think
>> it'd be
>> good to make sure that this change is desirable.  Can you update the
>>
> changelist
>
>> description to explain why you're doing what you're doing?  Is switching
>> to
>> FileWatcher solving an existing issue?
>>
>
>
>> http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc
>> File net/base/dns_reload_timer.cc (right):
>>
>
>
>
>
http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc#ne...
>
>> net/base/dns_reload_timer.cc:18: #include
>> "chrome/browser/file_path_watcher/file_path_watcher.h"
>> net is a lower level module than chrome.  chrome depends on net.  Not vice
>> versa.  So this dependency is unacceptable.
>>
>
> FileWatcher used to be part of base but it moved at some point. When I'm
> near
> git/svn again I'll try to see why. Perhaps it can move back to base or
> alternately this code may need to use inotify directly. There are also
> non-notify options to solve the underlying issue such as stat as mentioned
> in
> the bug but that's "expensive" to do on each lookup  but it will be much
> less
> code/complex than inotify.


It got moved because of the dependency on ChromeThread (now, BrowserThread).

It's possible to write a FileWatcher without this dependency, and add a
BrowserFileWatcher that uses BrowserThread to verify it's happening on the
file thread.  That's probably the right strategy here.


>
> http://codereview.chromium.org/6033006/
>

[willchan no longer on Chromium, 2011-01-05 20:37:24]

Thanks for your comments!  Unfortunately, I believe that the guidance provided
in the bug thread was made without a full understanding of the Chromium network
stack, and thus, the solution would result in suboptimal networking
interactions.  I'm sorry that I did not notice the bug thread and get involved
earlier before you wrote up this changelist.  Let's discuss it further on the
bug thread and put the change on hold for now.  I'll update the bug thread
shortly.

PS: Some meta-comments on this chromium change wrt conventions:
* Please add a line in the changelist description saying BUG=12345, since our
code review webapp (Rietveld) and commit bot will parse that and do various
things (linkify in the webapp and update the bug thread when the change is
committed).
* Please reply inline to reviewer comments.  For example,
http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc#ne....

On 2011/01/05 00:07:12, rojer9 wrote:
> On 2011/01/04 23:50:40, willchan wrote:
> > Drive-by comments.
> > 
> > Thanks for working on this change!  Can you motivate the change a bit more?
> 
> this is my proposed solution for http://crbug.com/67734
> the gist of it: resolv.conf needs to be re-read and current solution -
> re-reading on failure - doesn't work in some network configurations.
> watching file for changes and reloading resolv.conf if it has changed *before*
> attempting resolution solves that problem and is generally a better sound
> approach. though in this CL i'm not getting rid of the old one, my expectation
> is that it's not going to matter anymore.
> 
> > Is there a reason you wish to use FileWatcher?
> 
> i wanted to use the inotify(7) interface and FileWatcher already provides a
> handy wrapper around it (except for the dependency, which needs to be
> addressed).
> 
> > Are you changing the platform defines to make this code only run on Linux
> systems?
> 
> WATCH_RESOLV_CONF is just a more readable shortcut for the expression that was
> being used. yes, as it stands now all of resolv.conf logic is only built on
> linux and possibly freebsd: windows and os x don't need it and openbsd does
not
> support thread-safe reload of resolver configuration, so it's disabled there.
> this is all in the original comments and i'm not changing anything substantial
> here, just replaced a hairy expression with more readable one.
> 
> > There are a lot of
> > stylistic things that are incorrect in this change, probably due to your
> > unfamiliarity with Chromium style.  Before delving into those, I think it'd
be
> > good to make sure that this change is desirable.  Can you update the
> changelist
> > description to explain why you're doing what you're doing?  Is switching to
> > FileWatcher solving an existing issue?
> 
> it does. i think the discussion on the bug should clarify what is being
> addressed, why and how.
> 
> > 
> > http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc
> > File net/base/dns_reload_timer.cc (right):
> > 
> >
>
http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc#ne...
> > net/base/dns_reload_timer.cc:18: #include
> > "chrome/browser/file_path_watcher/file_path_watcher.h"
> > net is a lower level module than chrome.  chrome depends on net.  Not vice
> > versa.  So this dependency is unacceptable.
> 
> i agree, so i surmised when i was adding it :)
> it would be a pity if inotify had to be re-written just because of the
> dependency problem. maybe filewatcher could be pulled up?
> alternatively, this code could be moved down to chrome/browser, but there
needs
> to be some way of communicating with resolver code in net/base.

[Craig, 2011-01-06 03:42:37]

On 2011/01/05 20:37:24, willchan wrote:
> Thanks for your comments!  Unfortunately, I believe that the guidance provided
> in the bug thread was made without a full understanding of the Chromium
network
> stack, and thus, the solution would result in suboptimal networking
> interactions.  

Yeah that's my fault, sorry but I did suggest in the bug that we get agreement
from people familiar with the stack on the right approach before proceeding. The
pointer to the existing inotify code was intended to be a 'maybe this can be
used suggestion but I'm not sure' not 'you must use this'.


> I'm sorry that I did not notice the bug thread and get involved
> earlier before you wrote up this changelist.  Let's discuss it further on the
> bug thread and put the change on hold for now.  I'll update the bug thread
> shortly.
> 
> PS: Some meta-comments on this chromium change wrt conventions:
> * Please add a line in the changelist description saying BUG=12345, since our
> code review webapp (Rietveld) and commit bot will parse that and do various
> things (linkify in the webapp and update the bug thread when the change is
> committed).
> * Please reply inline to reviewer comments.  For example,
>
http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc#ne....
> 
> On 2011/01/05 00:07:12, rojer9 wrote:
> > On 2011/01/04 23:50:40, willchan wrote:
> > > Drive-by comments.
> > > 
> > > Thanks for working on this change!  Can you motivate the change a bit
more?
> > 
> > this is my proposed solution for http://crbug.com/67734
> > the gist of it: resolv.conf needs to be re-read and current solution -
> > re-reading on failure - doesn't work in some network configurations.
> > watching file for changes and reloading resolv.conf if it has changed
*before*
> > attempting resolution solves that problem and is generally a better sound
> > approach. though in this CL i'm not getting rid of the old one, my
expectation
> > is that it's not going to matter anymore.
> > 
> > > Is there a reason you wish to use FileWatcher?
> > 
> > i wanted to use the inotify(7) interface and FileWatcher already provides a
> > handy wrapper around it (except for the dependency, which needs to be
> > addressed).
> > 
> > > Are you changing the platform defines to make this code only run on Linux
> > systems?
> > 
> > WATCH_RESOLV_CONF is just a more readable shortcut for the expression that
was
> > being used. yes, as it stands now all of resolv.conf logic is only built on
> > linux and possibly freebsd: windows and os x don't need it and openbsd does
> not
> > support thread-safe reload of resolver configuration, so it's disabled
there.
> > this is all in the original comments and i'm not changing anything
substantial
> > here, just replaced a hairy expression with more readable one.
> > 
> > > There are a lot of
> > > stylistic things that are incorrect in this change, probably due to your
> > > unfamiliarity with Chromium style.  Before delving into those, I think
it'd
> be
> > > good to make sure that this change is desirable.  Can you update the
> > changelist
> > > description to explain why you're doing what you're doing?  Is switching
to
> > > FileWatcher solving an existing issue?
> > 
> > it does. i think the discussion on the bug should clarify what is being
> > addressed, why and how.
> > 
> > > 
> > > http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc
> > > File net/base/dns_reload_timer.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc#ne...
> > > net/base/dns_reload_timer.cc:18: #include
> > > "chrome/browser/file_path_watcher/file_path_watcher.h"
> > > net is a lower level module than chrome.  chrome depends on net.  Not vice
> > > versa.  So this dependency is unacceptable.
> > 
> > i agree, so i surmised when i was adding it :)
> > it would be a pity if inotify had to be re-written just because of the
> > dependency problem. maybe filewatcher could be pulled up?
> > alternatively, this code could be moved down to chrome/browser, but there
> needs
> > to be some way of communicating with resolver code in net/base.

[willchan no longer on Chromium, 2011-01-06 03:57:51]

On Wed, Jan 5, 2011 at 7:42 PM, <craig.schlenter@chromium.org> wrote:

> On 2011/01/05 20:37:24, willchan wrote:
>
>> Thanks for your comments!  Unfortunately, I believe that the guidance
>> provided
>> in the bug thread was made without a full understanding of the Chromium
>>
> network
>
>> stack, and thus, the solution would result in suboptimal networking
>> interactions.
>>
>
> Yeah that's my fault, sorry but I did suggest in the bug that we get
> agreement
> from people familiar with the stack on the right approach before
> proceeding. The
> pointer to the existing inotify code was intended to be a 'maybe this can
> be
> used suggestion but I'm not sure' not 'you must use this'.


Hah, don't worry Craig.  You already know a remarkable amount for someone
who only dabbles with Chromium in his spare time.  I wasn't blaming you.
 Was more apologizing to rojer9 for suggesting that, despite his helpful
work, we should probably revisit how we tackle this problem, and giving him
an explanation for why.


>
>
>
>  I'm sorry that I did not notice the bug thread and get involved
>> earlier before you wrote up this changelist.  Let's discuss it further on
>> the
>> bug thread and put the change on hold for now.  I'll update the bug thread
>> shortly.
>>
>
>  PS: Some meta-comments on this chromium change wrt conventions:
>> * Please add a line in the changelist description saying BUG=12345, since
>> our
>> code review webapp (Rietveld) and commit bot will parse that and do
>> various
>> things (linkify in the webapp and update the bug thread when the change is
>> committed).
>> * Please reply inline to reviewer comments.  For example,
>>
>
>
>
http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc#ne...
> .
>
>  On 2011/01/05 00:07:12, rojer9 wrote:
>> > On 2011/01/04 23:50:40, willchan wrote:
>> > > Drive-by comments.
>> > >
>> > > Thanks for working on this change!  Can you motivate the change a bit
>>
> more?
>
>> >
>> > this is my proposed solution for http://crbug.com/67734
>> > the gist of it: resolv.conf needs to be re-read and current solution -
>> > re-reading on failure - doesn't work in some network configurations.
>> > watching file for changes and reloading resolv.conf if it has changed
>>
> *before*
>
>> > attempting resolution solves that problem and is generally a better
>> sound
>> > approach. though in this CL i'm not getting rid of the old one, my
>>
> expectation
>
>> > is that it's not going to matter anymore.
>> >
>> > > Is there a reason you wish to use FileWatcher?
>> >
>> > i wanted to use the inotify(7) interface and FileWatcher already
>> provides a
>> > handy wrapper around it (except for the dependency, which needs to be
>> > addressed).
>> >
>> > > Are you changing the platform defines to make this code only run on
>> Linux
>> > systems?
>> >
>> > WATCH_RESOLV_CONF is just a more readable shortcut for the expression
>> that
>>
> was
>
>> > being used. yes, as it stands now all of resolv.conf logic is only built
>> on
>> > linux and possibly freebsd: windows and os x don't need it and openbsd
>> does
>> not
>> > support thread-safe reload of resolver configuration, so it's disabled
>>
> there.
>
>> > this is all in the original comments and i'm not changing anything
>>
> substantial
>
>> > here, just replaced a hairy expression with more readable one.
>> >
>> > > There are a lot of
>> > > stylistic things that are incorrect in this change, probably due to
>> your
>> > > unfamiliarity with Chromium style.  Before delving into those, I think
>>
> it'd
>
>> be
>> > > good to make sure that this change is desirable.  Can you update the
>> > changelist
>> > > description to explain why you're doing what you're doing?  Is
>> switching
>>
> to
>
>> > > FileWatcher solving an existing issue?
>> >
>> > it does. i think the discussion on the bug should clarify what is being
>> > addressed, why and how.
>> >
>> > >
>> > >
>> http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc
>> > > File net/base/dns_reload_timer.cc (right):
>> > >
>> > >
>> >
>>
>
>
>
http://codereview.chromium.org/6033006/diff/1/net/base/dns_reload_timer.cc#ne...
>
>> > > net/base/dns_reload_timer.cc:18: #include
>> > > "chrome/browser/file_path_watcher/file_path_watcher.h"
>> > > net is a lower level module than chrome.  chrome depends on net.  Not
>> vice
>> > > versa.  So this dependency is unacceptable.
>> >
>> > i agree, so i surmised when i was adding it :)
>> > it would be a pity if inotify had to be re-written just because of the
>> > dependency problem. maybe filewatcher could be pulled up?
>> > alternatively, this code could be moved down to chrome/browser, but
>> there
>> needs
>> > to be some way of communicating with resolver code in net/base.
>>
>
>
>
> http://codereview.chromium.org/6033006/
>