sync: abort active HTTP requests on shutdown.

sync: abort active HTTP requests on shutdown.

Renames ServerConnectionManager's 'Post' class to 'Connection'.

BUG=93829, 19757
TEST=SyncAPIServerConnectionManagerTest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99157

[tim (not reviewing), 2011-08-30 22:58:56]

Still adding tests to the new SyncAPIServerConnectionManagerTest for CheckTime,
and for additional race scenario, but the code is ready for a look.

Fred, please review everything.
Lingesh, I changed NonThreadSafe in SCM because we need to call TerminateAllIO
from a different thread, so this is mostly FYI, but feel free to review.

Thanks!

[akalin, 2011-08-31 07:20:39]

http://codereview.chromium.org/7792022/diff/4002/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/net/server_connection_manager.cc (right):

http://codereview.chromium.org/7792022/diff/4002/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/net/server_connection_manager.cc:207: // to Abort and
assignment of active_connection_.
I don't get it.  What race?  This still seems racy, since the AutoReset goes out
of scope without the lock being held.

http://codereview.chromium.org/7792022/diff/4002/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/net/server_connection_manager.cc:210: {
I suggest making MakeConnection() (or some non-virtual wrapper function):

- Hold the lock
- Check the terminated flag, and return NULL if terminated
- Otherwise, create a post and set active connection to it
- Return the post

also have a ClearActiveConnection() helper function that:
- holds the lock
- Clears active_connection_

Then you can just do:

scoped_ptr<Connection> post(MakeActiveConnection());

if (!post.get()) {
  ...
  return false;
}

bool ok = post->Init(...);

...

http://codereview.chromium.org/7792022/diff/4002/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/net/server_connection_manager.cc:226: bool ok =
active_connection_->Init(
clearer to do post->Init() here, I think

http://codereview.chromium.org/7792022/diff/4002/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/net/server_connection_manager.cc:257:
scoped_ptr<Connection> post;
similarly as above, you can just do:

scoped_ptr<Connection> post(MakeActiveConnection());
if (!post.get()) {
  break;
}

bool ok = post->Init(...);

[tim (not reviewing), 2011-08-31 14:46:17]

http://codereview.chromium.org/7792022/diff/4002/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/net/server_connection_manager.cc (right):

http://codereview.chromium.org/7792022/diff/4002/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/net/server_connection_manager.cc:207: // to Abort and
assignment of active_connection_.
On 2011/08/31 07:20:39, akalin wrote:
> I don't get it.  What race?  This still seems racy, since the AutoReset goes
out
> of scope without the lock being held.

We have to make sure that if terminate is called, we either have an
active_connection_ set and we're using it in here in an aborted state, or we
have not yet created it so we can bail out before we create.  The AutoReset was
so that I didn't have to add a Clear / reset... call at every early return point
from this function, which you'd have to do if you had a ClearActiveConnection. 
You could maybe just call that after Init, but we still interact with the post
after that point already so that seems weird.

An alternative is wiring up a call to Clear... in the dtor for Connection.  I
can look into that.

[tim (not reviewing), 2011-08-31 16:58:54]

Addressed comments and added test.  I took your idea of MakeActiveConnection,
and then added an OnConnectionDestroyed for the cleanup issue.

Good catch on the clearing of active_connection_!
When I said "still working on tests for race conditions", I was thinking stuff
like that + testing the case where we don't create an active connection because
we're terminated, but you made life easier :)

[akalin, 2011-08-31 21:10:03]

LGTM after comments below

http://codereview.chromium.org/7792022/diff/9001/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/net/server_connection_manager.cc (right):

http://codereview.chromium.org/7792022/diff/9001/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/net/server_connection_manager.cc:194: if
(active_connection_ != connection)
can't this be DCHECK_EQ(active_connection_, connection)?

http://codereview.chromium.org/7792022/diff/9001/chrome/browser/sync/internal...
File
chrome/browser/sync/internal_api/syncapi_server_connection_manager_unittest.cc
(right):

http://codereview.chromium.org/7792022/diff/9001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/syncapi_server_connection_manager_unittest.cc:30:
virtual void SetUserAgent(const char* user_agent) OVERRIDE {}
#include "base/compiler_specific.h" for override

http://codereview.chromium.org/7792022/diff/9001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/syncapi_server_connection_manager_unittest.cc:121:
base::Bind(&CallTerminateAllIO, &server),
why not base::Bind(&ServerConnectionManager::TerminateAllIO,
base::Unretained(&server))?

[tim (not reviewing), 2011-08-31 21:32:09]

Made changes, planning on committing this.

http://codereview.chromium.org/7792022/diff/9001/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/net/server_connection_manager.cc (right):

http://codereview.chromium.org/7792022/diff/9001/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/net/server_connection_manager.cc:194: if
(active_connection_ != connection)
On 2011/08/31 21:10:03, akalin wrote:
> can't this be DCHECK_EQ(active_connection_, connection)?

active_connection_ can be NULL already if it was aborted.  I could make this a
dual-condition dcheck.

I originally had this as just ClearActiveConnection(), called from the dtor from
Connection, but that seemed like a confused design / assumption made by the
'Connection' class, so I made it more general and put the logic in SCM, which is
what sets the active connection anyway.

[akalin, 2011-08-31 21:36:48]

On 2011/08/31 21:32:09, timsteele wrote:
> Made changes, planning on committing this.

Can you upload the latest changes for posterity?

>
http://codereview.chromium.org/7792022/diff/9001/chrome/browser/sync/engine/n...
> File chrome/browser/sync/engine/net/server_connection_manager.cc (right):
> 
>
http://codereview.chromium.org/7792022/diff/9001/chrome/browser/sync/engine/n...
> chrome/browser/sync/engine/net/server_connection_manager.cc:194: if
> (active_connection_ != connection)
> On 2011/08/31 21:10:03, akalin wrote:
> > can't this be DCHECK_EQ(active_connection_, connection)?
> 
> active_connection_ can be NULL already if it was aborted.  I could make this a
> dual-condition dcheck.

Oh, right.  Yeah, why don't you do:

if (active_connection_) {
  DCHECK_EQ(active_connection_, connection);
}

[commit-bot: I haz the power, 2011-08-31 21:38:27]

Can't apply patch for file
chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc.
While running patch -p1 --forward --force;
patching file
chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc
Hunk #1 FAILED at 8.
Hunk #2 FAILED at 54.
Hunk #3 FAILED at 77.
Hunk #4 FAILED at 96.
4 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc.rej

[commit-bot: I haz the power, 2011-09-01 02:28:17]

Try job failure for 7792022-17008 (retry) (retry) (retry) on linux for step
"compile" (clobber build).
It's a second try, previously, step "base_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-09-01 06:22:32]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win, revision is 99120, job name
was 7792022-17008 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-09-01 08:04:50]

Change committed as 99157