sync: make ServerConnectionManager explicitly NonThreadSafe

chrome/browser/sync/engine/net/server_connection_manager.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/engine/net/server_connection_manager.h"
 
 #include <errno.h>
 
 #include <ostream>
 #include <string>
@@ skipping 114 matching lines @@
   int bytes_read = buffer_.length();
   CHECK(length <= bytes_read);
   out_buffer->assign(buffer_);
   return bytes_read;
 }
 
 ScopedServerStatusWatcher::ScopedServerStatusWatcher(
     ServerConnectionManager* conn_mgr, HttpResponse* response)
     : conn_mgr_(conn_mgr),
       response_(response),
-      reset_count_(conn_mgr->reset_count_),
       server_reachable_(conn_mgr->server_reachable_) {
   response->server_status = conn_mgr->server_status_;
 }
 
 ScopedServerStatusWatcher::~ScopedServerStatusWatcher() {
-  // Don't update the status of the connection if it has been reset.
-  // TODO(timsteele): Do we need this? Is this used by multiple threads?
-  if (reset_count_ != conn_mgr_->reset_count_)
-    return;
   if (conn_mgr_->server_status_ != response_->server_status) {

[lipalani1, 2011/08/24 20:35:28]

It is not a big deal if it is not easy. But we mush have thread checking inside
this class too. Particularly inside the destructor.

[tim (not reviewing), 2011/08/25 16:21:46]

Good idea, looking into it.

     conn_mgr_->server_status_ = response_->server_status;
     conn_mgr_->NotifyStatusChanged();
     return;
   }
   // Notify if we've gone on or offline.
   if (server_reachable_ != conn_mgr_->server_reachable_)
     conn_mgr_->NotifyStatusChanged();
 }
 
 ServerConnectionManager::ServerConnectionManager(
     const string& server,
     int port,
     bool use_ssl,
     const string& user_agent)
     : sync_server_(server),
       sync_server_port_(port),
       user_agent_(user_agent),
       use_ssl_(use_ssl),
       proto_sync_path_(kSyncServerSyncPath),
       get_time_path_(kSyncServerGetTimePath),
       error_count_(0),
-      listeners_(new ObserverListThreadSafe<ServerConnectionEventListener>()),
       server_status_(HttpResponse::NONE),
-      server_reachable_(false),
-      reset_count_(0),
-      terminate_all_io_(false) {
+      server_reachable_(false) {
 }
 
 ServerConnectionManager::~ServerConnectionManager() {

[lipalani1, 2011/08/24 20:35:28]

Can you add a CHECK here?(rather than a DCHECK. Issues similar to this has been
hard to debug in the past.

[tim (not reviewing), 2011/08/25 16:21:46]

My issue with this is that NonThreadSafe by default has an assertion in the dtor
(which we get by inheriting), but it's a dcheck in accordance with the note
about CHECK vs DCHECK at http://dev.chromium.org/developers/coding-style 

I can add it if you insist, but I'm not sure we want / need to deviate here.  

 }
 
 void ServerConnectionManager::NotifyStatusChanged() {
-  listeners_->Notify(&ServerConnectionEventListener::OnServerConnectionEvent,
-      ServerConnectionEvent(server_status_, server_reachable_));
+  DCHECK(CalledOnValidThread());
+  FOR_EACH_OBSERVER(ServerConnectionEventListener, listeners_,
+     OnServerConnectionEvent(
+         ServerConnectionEvent(server_status_, server_reachable_)));
 }
 
 bool ServerConnectionManager::PostBufferWithCachedAuth(
     const PostBufferParams* params, ScopedServerStatusWatcher* watcher) {
+  DCHECK(CalledOnValidThread());
   string path =
       MakeSyncServerPath(proto_sync_path(), MakeSyncQueryString(client_id_));
   return PostBufferToPath(params, path, auth_token(), watcher);
 }
 
 bool ServerConnectionManager::PostBufferToPath(const PostBufferParams* params,
     const string& path, const string& auth_token,
     ScopedServerStatusWatcher* watcher) {
+  DCHECK(CalledOnValidThread());
   DCHECK(watcher != NULL);
 
   if (auth_token.empty()) {
     params->response->server_status = HttpResponse::SYNC_AUTH_ERROR;
     return false;
   }
 
   scoped_ptr<Post> post(MakePost());
   post->set_timing_info(params->timing_info);
   bool ok = post->Init(path.c_str(), auth_token, params->buffer_in,
@@ skipping 10 matching lines @@
 
   if (post->ReadBufferResponse(params->buffer_out, params->response, true)) {
     params->response->server_status = HttpResponse::SERVER_CONNECTION_OK;
     server_reachable_ = true;
     return true;
   }
   return false;
 }
 
 bool ServerConnectionManager::CheckTime(int32* out_time) {
+  DCHECK(CalledOnValidThread());
   // Verify that the server really is reachable by checking the time. We need
   // to do this because of wifi interstitials that intercept messages from the
   // client and return HTTP OK instead of a redirect.
   HttpResponse response;
   ScopedServerStatusWatcher watcher(this, &response);
   string post_body = "command=get_time";
 
-  // We only retry the CheckTime call if we were reset during the CheckTime
-  // attempt. We only try 3 times in case we're in a reset loop elsewhere.
-  base::subtle::AtomicWord start_reset_count = reset_count_ - 1;
-  for (int i = 0 ; i < 3 && start_reset_count != reset_count_ ; i++) {
-    start_reset_count = reset_count_;
+  for (int i = 0 ; i < 3;  i++) {
     scoped_ptr<Post> post(MakePost());
 
     // Note that the server's get_time path doesn't require authentication.
     string get_time_path =
         MakeSyncServerPath(kSyncServerGetTimePath, post_body);
     VLOG(1) << "Requesting get_time from:" << get_time_path;
 
     string blank_post_body;
     bool ok = post->Init(get_time_path.c_str(), blank_post_body,
         blank_post_body, &response);
@@ skipping 13 matching lines @@
     }
     *out_time = atoi(time_response.c_str());
     VLOG(1) << "Server was reachable.";
     return true;
   }
   IncrementErrorCount();
   return false;
 }
 
 bool ServerConnectionManager::IsServerReachable() {
+  DCHECK(CalledOnValidThread());
   int32 time;
   return CheckTime(&time);
 }
 
 bool ServerConnectionManager::IsUserAuthenticated() {
+  DCHECK(CalledOnValidThread());
   return IsGoodReplyFromServer(server_status_);
 }
 
 bool ServerConnectionManager::CheckServerReachable() {
+  DCHECK(CalledOnValidThread());
   const bool server_is_reachable = IsServerReachable();
   if (server_reachable_ != server_is_reachable) {
     server_reachable_ = server_is_reachable;
     NotifyStatusChanged();
   }
   return server_is_reachable;
 }
 
-void ServerConnectionManager::kill() {
-  {
-    base::AutoLock lock(terminate_all_io_mutex_);
-    terminate_all_io_ = true;
-  }
-}
-
-void ServerConnectionManager::ResetConnection() {
-  base::subtle::NoBarrier_AtomicIncrement(&reset_count_, 1);
-}
-
 bool ServerConnectionManager::IncrementErrorCount() {
-  error_count_mutex_.Acquire();
+  DCHECK(CalledOnValidThread());
   error_count_++;
 
   if (error_count_ > kMaxConnectionErrorsBeforeReset) {
     error_count_ = 0;
 
-    // Be careful with this mutex because calling out to other methods can
-    // result in being called back. Unlock it here to prevent any potential
-    // double-acquisitions.
-    error_count_mutex_.Release();
-
     if (!IsServerReachable()) {
       LOG(WARNING) << "Too many connection failures, server is not reachable. "
                    << "Resetting connections.";
-      ResetConnection();
     } else {
       LOG(WARNING) << "Multiple connection failures while server is reachable.";
     }
     return false;
   }
 
-  error_count_mutex_.Release();
   return true;
 }
 
 void ServerConnectionManager::SetServerParameters(const string& server_url,
                                                   int port,
                                                   bool use_ssl) {

[lipalani1, 2011/08/24 20:35:28]

Add dcheck here too.

[tim (not reviewing), 2011/08/25 16:21:46]

Done.

-  {
-    base::AutoLock lock(server_parameters_mutex_);
-    sync_server_ = server_url;
-    sync_server_port_ = port;
-    use_ssl_ = use_ssl;
-  }
+  sync_server_ = server_url;
+  sync_server_port_ = port;
+  use_ssl_ = use_ssl;
 }
 
 // Returns the current server parameters in server_url and port.
 void ServerConnectionManager::GetServerParameters(string* server_url,
                                                   int* port,
                                                   bool* use_ssl) const {
-  base::AutoLock lock(server_parameters_mutex_);
   if (server_url != NULL)
     *server_url = sync_server_;
   if (port != NULL)
     *port = sync_server_port_;
   if (use_ssl != NULL)
     *use_ssl = use_ssl_;
 }
 
 std::string ServerConnectionManager::GetServerHost() const {
   string server_url;
   int port;
   bool use_ssl;
   GetServerParameters(&server_url, &port, &use_ssl);
   // For unit tests.
   if (server_url.empty())
     return std::string();
   // We just want the hostname, so we don't need to switch on use_ssl.
   server_url = "http://" + server_url;
   GURL gurl(server_url);
   DCHECK(gurl.is_valid()) << gurl;
   return gurl.host();
 }
 
 void ServerConnectionManager::AddListener(
     ServerConnectionEventListener* listener) {

[lipalani1, 2011/08/24 20:35:28]

here too.

[tim (not reviewing), 2011/08/25 16:21:46]

Done.

-  listeners_->AddObserver(listener);
+  listeners_.AddObserver(listener);
 }
 
 void ServerConnectionManager::RemoveListener(
     ServerConnectionEventListener* listener) {

[lipalani1, 2011/08/24 20:35:28]

Here too.

[tim (not reviewing), 2011/08/25 16:21:46]

Done.

-  listeners_->RemoveObserver(listener);
+  listeners_.RemoveObserver(listener);
 }
 
 ServerConnectionManager::Post* ServerConnectionManager::MakePost() {
   return NULL;  // For testing.
 }
 
 bool FillMessageWithShareDetails(sync_pb::ClientToServerMessage* csm,
                                  syncable::DirectoryManager* manager,
                                  const std::string& share) {
   syncable::ScopedDirLookup dir(manager, share);
   if (!dir.good()) {
     VLOG(1) << "Dir lookup failed";
     return false;
   }
   string birthday = dir->store_birthday();
   if (!birthday.empty())
     csm->set_store_birthday(birthday);
   csm->set_share(share);
   return true;
 }
 
 std::ostream& operator << (std::ostream& s, const struct HttpResponse& hr) {
   s << " Response Code (bogus on error): " << hr.response_code;
   s << " Content-Length (bogus on error): " << hr.content_length;
   s << " Server Status: " << hr.server_status;
   return s;
 }
 
 }  // namespace browser_sync