Mutual exclusion between gconf and the glib main loop.

Mutual exclusion between gconf and the glib main loop.

BUG=11111

[sdoyon, 2009-04-28 17:34:47]

Bug and explanation at http://crbug.com/11111

This isn't very nice, but last time I asked by E-mail and at the
meeting, nobody appeared to have a better solution :-).

Thanks

[Evan Stade, 2009-04-28 18:09:46]

looks alright to me, but Dean probably has an opinion

[eroman, 2009-04-28 20:17:07]

<speaking_as_a_non-gconfg_expert>

I wander if it wouldn't be simpler to instead move the gconf calls to run on the
UI loop. Something along the lines of:

(1) Register a notification callback with gconf in the [UI thread] for when
proxy settings change (or do polling in the [UI thread] if there is no
notification mechanism).

(2) Once the notification callback fires on the [UI thread], post a task to the
[IO thread] to inform ProxyConfigServiceLinux (which runs on [IO thread]).

(3) When ProxyConfigServiceLinux receives a change notification, it updates its
cache, setting the config as its current value.

Now whenever ProxyConfigServiceLinux is polled for the current proxy config, it
just returns the current cached value (no other work necessary, no locks).

The edge case will be getting this sucker initialized. The very first call to
ProxyConfigServiceLinux::GetProxyConfig() should return a fresh config (and not
need to wait asynchronously for initial notification).

Note that from the perspective of ProxyService, it is fine for
ProxyConfigService to have an extra lag in noticing settings changes. (Just so
long as the initial one is fresh).

</speaking_as_a_non-gconfg_expert>

[sdoyon, 2009-04-29 14:57:19]

On Tue, 28 Apr 2009, eroman@chromium.org wrote:

> <speaking_as_a_non-gconfg_expert>

Yeah I don't think we have any of those :-).

> I wander if it wouldn't be simpler to instead move the gconf calls to
> run on the UI loop.

I see how that might be cleaner than sharing a lock between the glib
message pump and proxy service. OTOH it seems fairly complex to setup,
and the net effect is the same. (It's frustrating to have to have the
UI block on this work just because we can't isolate the gconf
callbacks, which are probably modest).

> The edge case will be getting this sucker initialized.

Where would you suggest I hookup something to get the UI thread to do
the initial reading of the gconf settings?

> The very first
> call to ProxyConfigServiceLinux::GetProxyConfig() should return a fresh
> config (and not need to wait asynchronously for initial notification).

AFAICT at that point GetProxyConfig() would have to block and wait
synchronously for the config to be available. I don't really know how
interdependent these threads are and whether this is advisible.

Would you have GetProxyConfig() post a task to the UI thread to get
the settings while the IO thread waits? Or would there be a good place
in the startup sequence to have the UI thread fetch initial gconf
settings proactively?

[eroman, 2009-04-30 20:31:24]

> OK, I'll need some hints to get started as to
> how/where to hook this up into the UI thread.
> See questions in previous message.

I am attaching some pseudo-code to illustrate how this could fit.

To handle the first-time initialization the easiest thing is to do it
synchronously during the startup critical path (the gconf call should be pretty
damn fast right?).

For this, you can put the code into ProxyService::CreateUsingV8Resolver(), as
this gets called from the main browser thread.

--------------------------

ProxyService* ProxyService::CreateUsingV8Resolver(
    const ProxyInfo* pi, URLRequestContext* url_request_context) {
< ... truncated ... >
#elif defined(OS_LINUX)
  config_service = new ProxyConfigService();

  // Assume we got called from the UI loop, so the current thread
  // is where we should be running gconf calls from.
  MessageLoop* glib_loop = MessageLoop::current();
  MessageLoop* io_loop = ...;

  // Synchronously fetch the current proxy config (since we are running
  // on glib_loop). Additionally start a notification system in |glib_loop|
  // to keep us updated on when the proxy config has changed.
  config_service->InitializeAndStartPolling(glib_loop, io_loop);
#endif
< ... truncated ... >
}

int ProxyConfigServiceLinux::GetProxyConfig(ProxyConfig* config) {
  DCHECK(MessageLoop::current() == io_loop_);

  // Simply return the last proxy configuration that glib_loop
  // notified us of (could be up to |poll_interval_ms_| milliseconds stale.
  config->Assign(last_config_);  // weirdness with ids to keep in mind...
  return last_config_result_;
}

void ProxyConfigServiceLinux::InitializeAndStartPolling(
    MessageLoop* glib_loop,
    MessageLoop* io_loop) {
  // We should be running on the glib thread right now.
  DCHECK(MessageLoop::current() == glib_loop);

  glib_loop_ = glib_loop;
  io_loop_ = io_loop;

  // Fetch and cache the current proxy config.
  last_config_result_ = GetLiveProxyConfig(&last_config_);

  // Start the configuration changes polling.
  PostCheckProxyConfigSettingsTask();
}

int ProxyConfigServiceLinux::GetLiveProxyConfig(ProxyConfig* config) {
  DCHECK(MessageLoop::current() == glib_loop);

  // This does the real work, as it is running on glib thread.
  // Basically this is the implementation currently in
  // ProxyConfigServiceLinux::GetProxyConfig().

  ...
}

void ProxyConfigServiceLinux::PostCheckProxyConfigSettingsTask() {
  DCHECK(MessageLoop::current() == glib_loop);

  // Start a recurring task that polls the proxy config.
  glib_loop_->PostDelayedTask(FROM_HERE, NewRunnableMethod(
      this, &ProxyConfigServiceLinux::OnCheckProxyConfigSettings),
      poll_interval_ms_);
}

void ProxyConfigServiceLinux::OnCheckProxyConfigSettings() {
  DCHECK(MessageLoop::current() == glib_loop);

  // Check what current settings actually are (by running gconfig stuff):
  ProxyConfig new_config;
  int new_config_result = GetLiveProxyConfig(&new_config);

  // See if it is different than what we had before.
  // Obviously you wouldn't be comparing against |last_config_| here,
  // since that data lives on the |io_loop|... but general idea is the same.
  if (new_config_result != last_config_result_ ||
      !new_config.Equals(last_config_))
    OnProxyConfigSettingsDidChange(new_config, new_config_result);

  // Continue the polling.
  PostCheckProxyConfigSettingsTask();
}

void ProxyConfigServiceLinux::OnProxyConfigSettingsDidChange(
    const ProxyConfig& new_config, int new_config_result) {
  DCHECK(MessageLoop::current() == glib_loop);

  // Post a task to |io_loop| with the new configuration, so it
  // can update |last_config_| and |last_config_result_|
  
  io_loop->PostTask(FROM_HERE, NewRunnableMethod(
      this, &ProxyConfigServiceLinux::SetNewProxyConfig,
      new_config, new_config_result));
}

void ProxyConfigServiceLinux::SetNewProxyConfig(
    const ProxyConfig& new_config, int new_config_result) {
  DCHECK(MessageLoop::current() == io_loop);

  last_config_.Assign(new_config);
  last_config_result = new_config_result;
}

--------------------------

A notable implementation caveat you will encounter trying to do this, is that
ProxyConfigServiceLinux needs to be ref-counted to use PostTask. That is
probably going to be a pain, so instead could make ProxyConfigServiceLinux a
light wrapper around a ref-counted implementation. Dunno.



> I'd just like to put this stuff behind me. Note the code currently checked in
is racy.

Heh, same here!

[sdoyon, 2009-05-01 15:59:40]

On Thu, 30 Apr 2009, eroman@chromium.org wrote:

>> OK, I'll need some hints to get started as to
>> how/where to hook this up into the UI thread.
>> See questions in previous message.
>
> I am attaching some pseudo-code to illustrate how this could fit.
>
> To handle the first-time initialization the easiest thing is to do it
> synchronously during the startup critical path (the gconf call should be
> pretty damn fast right?).

OK seems like 6ms is fairly typical. On one run it peaked to 50ms. If
I killall gconfd-2 beforehand and it needs to spawn a new one, it can
take 200-250ms, but that shouldn't be a normal case. In an earlier
experiment, it once seemed to hang for ~2s, but I couldn't reproduce
that so I'll assume it's a fluke.

> For this, you can put the code into
> ProxyService::CreateUsingV8Resolver(), as this gets called from the main
> browser thread.

Ah...! Great hint.

> config->Assign(last_config_);  // weirdness with ids to keep in
> mind...

What do you mean? Why can't we use operator=?

> void ProxyConfigServiceLinux::InitializeAndStartPolling(
>   MessageLoop* glib_loop,
>   MessageLoop* io_loop) {
> // We should be running on the glib thread right now.
> DCHECK(MessageLoop::current() == glib_loop);
>
> glib_loop_ = glib_loop;
> io_loop_ = io_loop;
>
> // Fetch and cache the current proxy config.
> last_config_result_ = GetLiveProxyConfig(&last_config_);

So IIUC the idea being that this is synchronous, so the IO thread
won't touch last_config_result_ until
ProxyService::CreateUsingV8Resolver() returns, right?

> // Start the configuration changes polling.
> PostCheckProxyConfigSettingsTask();

Or subscribe to notifications...

> A notable implementation caveat you will encounter trying to do this, is
> that ProxyConfigServiceLinux needs to be ref-counted to use PostTask.
> That is probably going to be a pain, so instead could make
> ProxyConfigServiceLinux a light wrapper around a ref-counted
> implementation. Dunno.

Is a ScopedRunnableMethodFactory not appropriate?

OK, I'll give this a shot. Thanks a lot for the pseudo-code.

[eroman, 2009-05-01 18:21:45]

> So IIUC the idea being that this is synchronous, so the IO thread
> won't touch last_config_result_ until
> ProxyService::CreateUsingV8Resolver() returns, right?

Correct. No network activity will start until after the URLRequestContext has
been initialized (so you don't need to worry about stuff running while
CreateUsingV8ProxyResolver() is executing).

> What do you mean? Why can't we use operator=?

The issue to consider here is that ProxyConfig has an internal ID field, which
gets initialized by the ctor using a static counter.

So if you use operator= you will overwrite the ID of the passed in object, which
the consumer may not be expecting.

And moreover, constructing from more than one thread is not safe because of the
static.

I will see if I can eliminate this dependency today so you don't need to worry
about it.

> Is a ScopedRunnableMethodFactory not appropriate?

Oh right, that is probably a better solution :-)
(/me is reminded to clean up some of my unit tests).

[eroman, 2009-05-06 05:39:33]

> Shall I pass it down from the browser, through 
> CreateUsingV8Resolver() from chrome_url_request_context.cc?
> Is that OK?

Sounds fine to me.

[sdoyon, 2009-05-06 19:32:41]

Superseded by http://codereview.chromium.org/113043