Skip to content

Commit

Permalink
[GCM] Suppress connection attempts when there is no valid connection.
Browse files Browse the repository at this point in the history 
If the local machine's network is unavailable, there's no poin in attempting to
make connections. This change simplifies the connection listening logic and
drops connection attempts while it is known that the network is unavailable.

BUG=376556

Review URL: https://codereview.chromium.org/336473003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@278662 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
zea@chromium.org committed Jun 20, 2014
1 parent bcca2f4 commit 644730f
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 48 deletions.
38 changes: 21 additions & 17 deletions google_apis/gcm/engine/connection_factory_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ ConnectionFactoryImpl::ConnectionFactoryImpl(
pac_request_(NULL),
connecting_(false),
waiting_for_backoff_(false),
waiting_for_network_online_(false),
logging_in_(false),
recorder_(recorder),
listener_(NULL),
Expand All @@ -65,8 +66,7 @@ ConnectionFactoryImpl::ConnectionFactoryImpl(
}

ConnectionFactoryImpl::~ConnectionFactoryImpl() {
net::NetworkChangeNotifier::RemoveIPAddressObserver(this);
net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
if (pac_request_) {
network_session_->proxy_service()->CancelPacRequest(pac_request_);
pac_request_ = NULL;
Expand All @@ -83,8 +83,8 @@ void ConnectionFactoryImpl::Initialize(
backoff_entry_ = CreateBackoffEntry(&backoff_policy_);
request_builder_ = request_builder;

net::NetworkChangeNotifier::AddIPAddressObserver(this);
net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
waiting_for_network_online_ = net::NetworkChangeNotifier::IsOffline();
connection_handler_ = CreateConnectionHandler(
base::TimeDelta::FromMilliseconds(kReadTimeoutMs),
read_callback,
Expand Down Expand Up @@ -150,6 +150,8 @@ std::string ConnectionFactoryImpl::GetConnectionStateString() const {
return "CONNECTING";
if (waiting_for_backoff_)
return "WAITING FOR BACKOFF";
if (waiting_for_network_online_)
return "WAITING FOR NETWORK CHANGE";
return "NOT CONNECTED";
}

Expand Down Expand Up @@ -182,6 +184,9 @@ void ConnectionFactoryImpl::SignalConnectionReset(
CloseSocket();
DCHECK(!IsEndpointReachable());

if (waiting_for_network_online_)
return;

// Network changes get special treatment as they can trigger a one-off canary
// request that bypasses backoff (but does nothing if a connection is in
// progress). Other connection reset events can be ignored as a connection
Expand All @@ -208,8 +213,6 @@ void ConnectionFactoryImpl::SignalConnectionReset(
// We shouldn't be in backoff in thise case.
DCHECK_EQ(0, backoff_entry_->failure_count());
}
DCHECK(!connecting_);
DCHECK(!waiting_for_backoff_);

// At this point the last login time has been consumed or deemed irrelevant,
// reset it.
Expand All @@ -229,21 +232,19 @@ base::TimeTicks ConnectionFactoryImpl::NextRetryAttempt() const {
return backoff_entry_->GetReleaseTime();
}

void ConnectionFactoryImpl::OnConnectionTypeChanged(
void ConnectionFactoryImpl::OnNetworkChanged(
net::NetworkChangeNotifier::ConnectionType type) {
if (type == net::NetworkChangeNotifier::CONNECTION_NONE)
if (type == net::NetworkChangeNotifier::CONNECTION_NONE) {
DVLOG(1) << "Network lost, resettion connection.";
waiting_for_network_online_ = true;

// Will do nothing due to |waiting_for_network_online_ == true|.
SignalConnectionReset(NETWORK_CHANGE);
return;
}

DVLOG(1) << "Connection type changed to " << type << ", reconnecting.";

// The connection may have been silently dropped, attempt to reconnect.
SignalConnectionReset(NETWORK_CHANGE);
}

void ConnectionFactoryImpl::OnIPAddressChanged() {
DVLOG(1) << "IP Address changed, reconnecting.";

// The connection may have been silently dropped, attempt to reconnect.
waiting_for_network_online_ = false;
SignalConnectionReset(NETWORK_CHANGE);
}

Expand Down Expand Up @@ -271,6 +272,9 @@ void ConnectionFactoryImpl::ConnectImpl() {
DCHECK(!IsEndpointReachable());
DCHECK(!socket_handle_.socket());

if (waiting_for_network_online_)
return;

connecting_ = true;
GURL current_endpoint = GetCurrentEndpoint();
recorder_->RecordConnectionInitiated(current_endpoint.host());
Expand Down
13 changes: 8 additions & 5 deletions google_apis/gcm/engine/connection_factory_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ class GCMStatsRecorder;

class GCM_EXPORT ConnectionFactoryImpl :
public ConnectionFactory,
public net::NetworkChangeNotifier::ConnectionTypeObserver,
public net::NetworkChangeNotifier::IPAddressObserver {
public net::NetworkChangeNotifier::NetworkChangeObserver {
public:
ConnectionFactoryImpl(
const std::vector<GURL>& mcs_endpoints,
Expand All @@ -53,10 +52,9 @@ class GCM_EXPORT ConnectionFactoryImpl :
virtual void SignalConnectionReset(ConnectionResetReason reason) OVERRIDE;
virtual void SetConnectionListener(ConnectionListener* listener) OVERRIDE;

// NetworkChangeNotifier observer implementations.
virtual void OnConnectionTypeChanged(
// NetworkChangeObserver implementation.
virtual void OnNetworkChanged(
net::NetworkChangeNotifier::ConnectionType type) OVERRIDE;
virtual void OnIPAddressChanged() OVERRIDE;

// Returns the server to which the factory is currently connected, or if
// a connection is currently pending, the server to which the next connection
Expand Down Expand Up @@ -149,6 +147,11 @@ class GCM_EXPORT ConnectionFactoryImpl :
// expiration.
bool waiting_for_backoff_;

// Whether the NetworkChangeNotifier has informed the client that there is
// no current connection. No connection attempts will be made until the
// client is informed of a valid connection type.
bool waiting_for_network_online_;

// Whether login successfully completed after the connection was established.
// If a connection reset happens while attempting to log in, the current
// backoff entry is reused (after incrementing with a new failure).
Expand Down
56 changes: 30 additions & 26 deletions google_apis/gcm/engine/connection_factory_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,35 +387,16 @@ TEST_F(ConnectionFactoryImplTest, MultipleFailuresThenSucceed) {
EXPECT_TRUE(connected_server().is_valid());
}

// IP events should trigger canary connections.
TEST_F(ConnectionFactoryImplTest, FailThenIPEvent) {
// Network change events should trigger canary connections.
TEST_F(ConnectionFactoryImplTest, FailThenNetworkChangeEvent) {
factory()->SetConnectResult(net::ERR_CONNECTION_FAILED);
factory()->Connect();
WaitForConnections();
base::TimeTicks initial_backoff = factory()->NextRetryAttempt();
EXPECT_FALSE(initial_backoff.is_null());

factory()->SetConnectResult(net::ERR_FAILED);
factory()->OnIPAddressChanged();
WaitForConnections();

// Backoff should increase.
base::TimeTicks next_backoff = factory()->NextRetryAttempt();
EXPECT_GT(next_backoff, initial_backoff);
EXPECT_FALSE(factory()->IsEndpointReachable());
}

// Connection type events should trigger canary connections.
TEST_F(ConnectionFactoryImplTest, FailThenConnectionTypeEvent) {
factory()->SetConnectResult(net::ERR_CONNECTION_FAILED);
factory()->Connect();
WaitForConnections();
base::TimeTicks initial_backoff = factory()->NextRetryAttempt();
EXPECT_FALSE(initial_backoff.is_null());

factory()->SetConnectResult(net::ERR_FAILED);
factory()->OnConnectionTypeChanged(
net::NetworkChangeNotifier::CONNECTION_WIFI);
factory()->OnNetworkChanged(net::NetworkChangeNotifier::CONNECTION_WIFI);
WaitForConnections();

// Backoff should increase.
Expand All @@ -434,8 +415,7 @@ TEST_F(ConnectionFactoryImplTest, CanarySucceedsThenDisconnects) {
EXPECT_FALSE(initial_backoff.is_null());

factory()->SetConnectResult(net::OK);
factory()->OnConnectionTypeChanged(
net::NetworkChangeNotifier::CONNECTION_WIFI);
factory()->OnNetworkChanged(net::NetworkChangeNotifier::CONNECTION_ETHERNET);
WaitForConnections();
EXPECT_TRUE(factory()->IsEndpointReachable());
EXPECT_TRUE(connected_server().is_valid());
Expand All @@ -460,8 +440,7 @@ TEST_F(ConnectionFactoryImplTest, CanarySucceedsRetryDuringLogin) {

factory()->SetDelayLogin(true);
factory()->SetConnectResult(net::OK);
factory()->OnConnectionTypeChanged(
net::NetworkChangeNotifier::CONNECTION_WIFI);
factory()->OnNetworkChanged(net::NetworkChangeNotifier::CONNECTION_WIFI);
WaitForConnections();
EXPECT_FALSE(factory()->IsEndpointReachable());

Expand Down Expand Up @@ -545,4 +524,29 @@ TEST_F(ConnectionFactoryImplTest, SignalResetRestoresBackoff) {
EXPECT_FALSE(connected_server().is_valid());
}

// When the network is disconnected, close the socket and suppress further
// connection attempts until the network returns.
TEST_F(ConnectionFactoryImplTest, SuppressConnectWhenNoNetwork) {
factory()->SetConnectResult(net::OK);
factory()->Connect();
EXPECT_TRUE(factory()->NextRetryAttempt().is_null());
EXPECT_TRUE(factory()->IsEndpointReachable());

// Advance clock so the login window reset isn't encountered.
factory()->tick_clock()->Advance(base::TimeDelta::FromSeconds(11));

// Will trigger reset, but will not attempt a new connection.
factory()->OnNetworkChanged(net::NetworkChangeNotifier::CONNECTION_NONE);
EXPECT_FALSE(factory()->IsEndpointReachable());
EXPECT_TRUE(factory()->NextRetryAttempt().is_null());

// When the network returns, attempt to connect.
factory()->SetConnectResult(net::OK);
factory()->OnNetworkChanged(net::NetworkChangeNotifier::CONNECTION_4G);
WaitForConnections();

EXPECT_TRUE(factory()->IsEndpointReachable());
EXPECT_TRUE(factory()->NextRetryAttempt().is_null());
}

} // namespace gcm

0 comments on commit 644730f

Please sign in to comment.