Skip to content

GitLab

T
tangled
Commit 4d296dec authored by Jimmy Gong's avatar Jimmy Gong Committed by Commit Bot
Browse files
Options

Revert "Add connection time outs to ConnectionManager" 

This reverts commit 1dbaf3e8.

Reason for revert: Noticing increased flake connections due to this CL

Original change's description:
> Add connection time outs to ConnectionManager
>
> If we cannot establish a connection within 15 seconds, we time out
> the connection and cancel
>
> Bug: 1106937
> Test: chromeos_components_unittests
> Change-Id: Iad731f6119a900fa411d75a9c53869baabf33498
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2470321
> Commit-Queue: Jimmy Gong <jimmyxgong@chromium.org>
> Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#817371}

TBR=khorimoto@chromium.org,jimmyxgong@chromium.org

Change-Id: I6377c38944ecbdb028e1697521ec104023909ecc
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1106937
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2477658Reviewed-by: default avatarJimmy Gong <jimmyxgong@chromium.org>
Reviewed-by: default avatarKyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Jimmy Gong <jimmyxgong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#817737}
parent 4f12b3e9
Hide whitespace changes
Inline Side-by-side
Showing with 5 additions and 91 deletions
chromeos/components/phonehub/connection_manager_impl.cc
View file @ 4d296dec
......@@ -5,7 +5,6 @@
#include "chromeos/components/phonehub/connection_manager_impl.h"
#include "base/bind_helpers.h"
#include "base/time/time.h"
#include "chromeos/constants/chromeos_features.h"
#include "chromeos/services/device_sync/public/cpp/device_sync_client.h"
#include "chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.h"
......@@ -14,33 +13,19 @@
namespace chromeos {
namespace phonehub {
namespace {
constexpr char kPhoneHubFeatureName[] = "phone_hub";
constexpr base::TimeDelta kConnectionTimeoutSeconds(
base::TimeDelta::FromSeconds(15u));
const char kPhoneHubFeatureName[] = "phone_hub";
} // namespace
ConnectionManagerImpl::ConnectionManagerImpl(
multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
device_sync::DeviceSyncClient* device_sync_client,
chromeos::secure_channel::SecureChannelClient* secure_channel_client) {
ConnectionManagerImpl(multidevice_setup_client_, device_sync_client,
secure_channel_client,
std::make_unique<base::OneShotTimer>());
}
ConnectionManagerImpl::ConnectionManagerImpl(
multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
device_sync::DeviceSyncClient* device_sync_client,
chromeos::secure_channel::SecureChannelClient* secure_channel_client,
std::unique_ptr<base::OneShotTimer> timer)
chromeos::secure_channel::SecureChannelClient* secure_channel_client)
: multidevice_setup_client_(multidevice_setup_client),
device_sync_client_(device_sync_client),
secure_channel_client_(secure_channel_client),
timer_(std::move(timer)) {
secure_channel_client_(secure_channel_client) {
DCHECK(multidevice_setup_client_);
DCHECK(device_sync_client_);
DCHECK(secure_channel_client_);
DCHECK(timer_);
}
ConnectionManagerImpl::~ConnectionManagerImpl() {
......@@ -96,10 +81,6 @@ void ConnectionManagerImpl::AttemptConnection() {
connection_attempt_->SetDelegate(this);
NotifyStatusChanged();
timer_->Start(FROM_HERE, kConnectionTimeoutSeconds,
base::BindOnce(&ConnectionManagerImpl::OnConnectionTimeout,
weak_ptr_factory_.GetWeakPtr()));
}
void ConnectionManagerImpl::SendMessage(const std::string& payload) {
......@@ -115,7 +96,6 @@ void ConnectionManagerImpl::OnConnectionAttemptFailure(
chromeos::secure_channel::mojom::ConnectionAttemptFailureReason reason) {
PA_LOG(WARNING) << "AttemptConnection() failed to establish connection with "
<< "error: " << reason << ".";
timer_->Stop();
connection_attempt_.reset();
NotifyStatusChanged();
}
......@@ -124,15 +104,12 @@ void ConnectionManagerImpl::OnConnection(
std::unique_ptr<chromeos::secure_channel::ClientChannel> channel) {
PA_LOG(VERBOSE) << "AttemptConnection() successfully established a "
<< "connection between local and remote device.";
timer_->Stop();
channel_ = std::move(channel);
channel_->AddObserver(this);
NotifyStatusChanged();
}
void ConnectionManagerImpl::OnDisconnected() {
// Stop timer in case we are disconnected before the connection timed out.
timer_->Stop();
connection_attempt_.reset();
channel_->RemoveObserver(this);
channel_.reset();
......@@ -143,13 +120,5 @@ void ConnectionManagerImpl::OnMessageReceived(const std::string& payload) {
NotifyMessageReceived(payload);
}
void ConnectionManagerImpl::OnConnectionTimeout() {
PA_LOG(WARNING) << "AttemptionConnection() has timed out. Closing connection "
<< "attempt.";
connection_attempt_.reset();
NotifyStatusChanged();
}
} // namespace phonehub
} // namespace chromeos
chromeos/components/phonehub/connection_manager_impl.h
View file @ 4d296dec
......@@ -7,9 +7,7 @@
#include <memory>
#include "base/memory/weak_ptr.h"
#include "base/optional.h"
#include "base/timer/timer.h"
#include "chromeos/components/phonehub/connection_manager.h"
#include "chromeos/services/secure_channel/public/cpp/client/client_channel.h"
#include "chromeos/services/secure_channel/public/cpp/client/connection_attempt.h"
......@@ -50,14 +48,6 @@ class ConnectionManagerImpl
void SendMessage(const std::string& payload) override;
private:
friend class ConnectionManagerImplTest;
ConnectionManagerImpl(
multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
device_sync::DeviceSyncClient* device_sync_client,
chromeos::secure_channel::SecureChannelClient* secure_channel_client,
std::unique_ptr<base::OneShotTimer> timer);
// chromeos::secure_channel::ConnectionAttempt::Delegate:
void OnConnectionAttemptFailure(
chromeos::secure_channel::mojom::ConnectionAttemptFailureReason reason)
......@@ -69,8 +59,6 @@ class ConnectionManagerImpl
void OnDisconnected() override;
void OnMessageReceived(const std::string& payload) override;
void OnConnectionTimeout();
multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client_;
device_sync::DeviceSyncClient* device_sync_client_;
......@@ -82,10 +70,6 @@ class ConnectionManagerImpl
connection_attempt_;
std::unique_ptr<chromeos::secure_channel::ClientChannel> channel_;
std::unique_ptr<base::OneShotTimer> timer_;
base::WeakPtrFactory<ConnectionManagerImpl> weak_ptr_factory_{this};
};
} // namespace phonehub
......
chromeos/components/phonehub/connection_manager_impl_unittest.cc
View file @ 4d296dec
......@@ -7,10 +7,7 @@
#include <memory>
#include "base/macros.h"
#include "base/memory/ptr_util.h"
#include "base/test/scoped_feature_list.h"
#include "base/time/time.h"
#include "base/timer/mock_timer.h"
#include "chromeos/components/multidevice/remote_device_test_util.h"
#include "chromeos/constants/chromeos_features.h"
#include "chromeos/services/device_sync/public/cpp/fake_device_sync_client.h"
......@@ -26,9 +23,6 @@ namespace {
using multidevice_setup::mojom::HostStatus;
constexpr base::TimeDelta kExpectedTimeoutSeconds(
base::TimeDelta::FromSeconds(15u));
class FakeObserver : public ConnectionManager::Observer {
public:
FakeObserver() = default;
......@@ -72,14 +66,12 @@ class ConnectionManagerImplTest : public testing::Test {
// testing::Test:
void SetUp() override {
auto timer = std::make_unique<base::MockOneShotTimer>();
mock_timer_ = timer.get();
fake_device_sync_client_.set_local_device_metadata(test_local_device_);
fake_multidevice_setup_client_.SetHostStatusWithDevice(
std::make_pair(HostStatus::kHostVerified, test_remote_device_));
connection_manager_ = base::WrapUnique(new ConnectionManagerImpl(
connection_manager_ = std::make_unique<ConnectionManagerImpl>(
&fake_multidevice_setup_client_, &fake_device_sync_client_,
fake_secure_channel_client_.get(), std::move(timer)));
fake_secure_channel_client_.get());
connection_manager_->AddObserver(&fake_observer_);
EXPECT_EQ(ConnectionManager::Status::kDisconnected, GetStatus());
}
......@@ -115,19 +107,6 @@ class ConnectionManagerImplTest : public testing::Test {
}
}
void VerifyTimerSet() {
EXPECT_TRUE(mock_timer_->IsRunning());
EXPECT_EQ(kExpectedTimeoutSeconds, mock_timer_->GetCurrentDelay());
}
void VerifyTimerStopped() { EXPECT_FALSE(mock_timer_->IsRunning()); }
void InvokeTimerTask() {
VerifyTimerSet();
mock_timer_->Fire();
}
base::MockOneShotTimer* mock_timer_;
chromeos::multidevice::RemoteDeviceRef test_remote_device_;
chromeos::multidevice::RemoteDeviceRef test_local_device_;
device_sync::FakeDeviceSyncClient fake_device_sync_client_;
......@@ -281,23 +260,5 @@ TEST_F(ConnectionManagerImplTest, SuccessfullyAttemptConnectionWithBle) {
EXPECT_EQ(ConnectionManager::Status::kConnected, GetStatus());
}
TEST_F(ConnectionManagerImplTest, ConnectionTimeout) {
CreateFakeConnectionAttempt();
connection_manager_->AttemptConnection();
// Status has been updated to connecting, verify that the status observer
// has been called.
EXPECT_EQ(1u, GetNumStatusObserverCalls());
EXPECT_EQ(ConnectionManager::Status::kConnecting, GetStatus());
VerifyTimerSet();
// Simulate fast forwarding time to time out the connection request.
InvokeTimerTask();
VerifyTimerStopped();
EXPECT_EQ(2u, GetNumStatusObserverCalls());
EXPECT_EQ(ConnectionManager::Status::kDisconnected, GetStatus());
}
} // namespace phonehub
} // namespace chromeos
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment