Clean-up/Bug-fix: Move all UdpSocketPosix impl out of public API.
1. Remove implementation code from the UdpSocket interface class.
2. Move any constructs related only to the standalone UdpSocketPosix
impl out of the UdpSocket class.
3. Bug fix a use-before-init bug when trying to read packets with
UdpSocketPosix. #1 implicitly fixed this bug, which was being caused by
LifetimeObserver::OnCreate() being called from the "interface" class
before the UdpSocketPosix::handle_ data member was initialized in the
UdpSocketPosix subclass.
4. Clean-up object graph permission details: Now, only
UdpSocketReaderPosix can set the global "lifetime observer" pointer; and
client code no longer needs to worry about this internal detail.
5. Isolated thread-aware code to just UdpSocketPosix::ReadMessage() and
UdpSocketPosix::Close(), with the rest of the implementation being
non-thread-safe, per OpenScreen's threading decisions. To be clear, the
entire UdpSocket API is not meant to be used by anything other than the
TaskRunner thread. Updated class-level comments to reflect this.
Change-Id: I1c7a27a8271ea856c4f247927333b1ba4f9fcabe
Reviewed-on: https://chromium-review.googlesource.com/c/openscreen/+/1869803
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Max Yakimakha <yakimakha@chromium.org>
Commit-Queue: Yuri Wiitala <miu@chromium.org>
diff --git a/platform/api/udp_socket.cc b/platform/api/udp_socket.cc
index 3c75b18..60262ca 100644
--- a/platform/api/udp_socket.cc
+++ b/platform/api/udp_socket.cc
@@ -4,82 +4,11 @@
#include "platform/api/udp_socket.h"
-#include "platform/api/task_runner.h"
-
namespace openscreen {
namespace platform {
-UdpSocket::UdpSocket(TaskRunner* task_runner, Client* client)
- : client_(client), task_runner_(task_runner) {
- OSP_CHECK(task_runner_);
- if (lifetime_observer_.load()) {
- lifetime_observer_.load()->OnCreate(this);
- }
-}
-
-UdpSocket::~UdpSocket() {
- OSP_DCHECK(is_closed_);
-}
-
-// static
-void UdpSocket::SetLifetimeObserver(LifetimeObserver* observer) {
- lifetime_observer_.store(observer);
-}
-
-// static
-std::atomic<UdpSocket::LifetimeObserver*> UdpSocket::lifetime_observer_{
- nullptr};
-
-void UdpSocket::OnError(Error error) {
- CloseIfError(error);
-
- if (!client_) {
- return;
- }
-
- task_runner_->PostTask([e = std::move(error), this]() mutable {
- // TODO(crbug.com/openscreen/71): |this| may be invalid at this point.
- this->client_->OnError(this, std::move(e));
- });
-}
-
-void UdpSocket::OnSendError(Error error) {
- if (!client_) {
- return;
- }
-
- task_runner_->PostTask([e = std::move(error), this]() mutable {
- // TODO(crbug.com/openscreen/71): |this| may be invalid at this point.
- this->client_->OnSendError(this, std::move(e));
- });
-}
-
-void UdpSocket::OnRead(ErrorOr<UdpPacket> read_data) {
- if (!client_) {
- return;
- }
-
- task_runner_->PostTask([data = std::move(read_data), this]() mutable {
- // TODO(crbug.com/openscreen/71): |this| may be invalid at this point.
- this->client_->OnRead(this, std::move(data));
- });
-}
-
-void UdpSocket::CloseIfError(const Error& error) {
- if (error.code() != Error::Code::kNone &&
- error.code() != Error::Code::kAgain) {
- CloseIfOpen();
- }
-}
-
-void UdpSocket::CloseIfOpen() {
- if (!is_closed_.exchange(true)) {
- if (lifetime_observer_.load()) {
- lifetime_observer_.load()->OnDestroy(this);
- }
- Close();
- }
-}
+UdpSocket::UdpSocket() = default;
+UdpSocket::~UdpSocket() = default;
} // namespace platform
} // namespace openscreen
diff --git a/platform/api/udp_socket.h b/platform/api/udp_socket.h
index ff9c033..f5d462a 100644
--- a/platform/api/udp_socket.h
+++ b/platform/api/udp_socket.h
@@ -31,28 +31,8 @@
// Usage: The socket is created and opened by calling the Create() method. This
// returns a unique pointer that auto-closes/destroys the socket when it goes
// out-of-scope.
-//
-// Platform implementation note: There must only be one platform-specific
-// implementation of UdpSocket linked into the library/application. For that
-// reason, none of the methods here are declared virtual (i.e., the overhead is
-// pure waste). However, UdpSocket can be subclassed to include all extra
-// private state, such as OS-specific handles. See UdpSocketPosix for a
-// reference implementation.
class UdpSocket {
public:
- virtual ~UdpSocket();
-
- class LifetimeObserver {
- public:
- virtual ~LifetimeObserver() = default;
-
- // Function to call upon creation of a new UdpSocket.
- virtual void OnCreate(UdpSocket* socket) = 0;
-
- // Function to call upon deletion of a UdpSocket.
- virtual void OnDestroy(UdpSocket* socket) = 0;
- };
-
// Client for the UdpSocket class.
class Client {
public:
@@ -88,23 +68,20 @@
kLowPriority = 0x20
};
- // The LifetimeObserver set here must exist during ANY future UdpSocket
- // creations. SetLifetimeObserver(nullptr) must be called before any future
- // socket creations on destructions after the observer is destroyed
- static void SetLifetimeObserver(LifetimeObserver* observer);
-
using Version = IPAddress::Version;
// Creates a new, scoped UdpSocket within the IPv4 or IPv6 family.
// |local_endpoint| may be zero (see comments for Bind()). This method must be
- // defined in the platform-level implementation. All client_ methods called
- // will be queued on the provided task_runner. For this reason, the provided
- // task_runner and client must exist for the duration of the created socket's
+ // defined in the platform-level implementation. All |client| methods called
+ // will be queued on the provided |task_runner|. For this reason, the provided
+ // TaskRunner and Client must exist for the duration of the created socket's
// lifetime.
static ErrorOr<UdpSocketUniquePtr> Create(TaskRunner* task_runner,
Client* client,
const IPEndpoint& local_endpoint);
+ virtual ~UdpSocket();
+
// Returns true if |socket| belongs to the IPv4/IPv6 address family.
virtual bool IsIPv4() const = 0;
virtual bool IsIPv6() const = 0;
@@ -117,7 +94,7 @@
// Binds to the address specified in the constructor. If the local endpoint's
// address is zero, the operating system will bind to all interfaces. If the
// local endpoint's port is zero, the operating system will automatically find
- // a free local port and bind to it. Future calls to local_endpoint() will
+ // a free local port and bind to it. Future calls to GetLocalEndpoint() will
// reflect the resolved port.
virtual void Bind() = 0;
@@ -129,8 +106,8 @@
virtual void JoinMulticastGroup(const IPAddress& address,
NetworkInterfaceIndex ifindex) = 0;
- // Sends a message and returns the number of bytes sent, on success.
- // Error::Code::kAgain might be returned to indicate the operation would
+ // Sends a message. If the message is not sent, Client::OnSendError() will be
+ // called to indicate this. Error::Code::kAgain indicates the operation would
// block, which can be expected during normal operation.
virtual void SendMessage(const void* data,
size_t length,
@@ -140,49 +117,9 @@
virtual void SetDscp(DscpMode state) = 0;
protected:
- // Creates a new UdpSocket. The provided client and task_runner must exist for
- // the duration of this socket's lifetime.
- UdpSocket(TaskRunner* task_runner, Client* client);
-
- // Methods to take care of posting UdpSocket::Client callbacks for client_ to
- // task_runner_.
- // NOTE: OnError(...) will close the socket in addition to returning the
- // error.
- void OnError(Error error);
- void OnSendError(Error error);
- void OnRead(ErrorOr<UdpPacket> read_data);
-
- // Closes an open socket when a non-recoverable error occurs.
- void CloseIfError(const Error& error);
-
- // Closes an open socket.
- // NOTE: Concrete implementations of UdpSocket must call this method in their
- // destructor.
- void CloseIfOpen();
-
- // Returns whether the socket is currently closed.
- // NOTE: This must be checked before calling any operation on the socket.
- bool is_closed() { return is_closed_.load(); }
+ UdpSocket();
private:
- static std::atomic<LifetimeObserver*> lifetime_observer_;
-
- // Closes this socket.
- // NOTE: This method will only be called once.
- virtual void Close() {}
-
- // Atomically keeps track of if the socket is closed, so that threading
- // across different implementations isn't a problem.
- std::atomic_bool is_closed_{false};
-
- // Client to use for callbacks.
- // NOTE: client_ can be nullptr if the user does not want any callbacks (for
- // example, in the send-only case).
- Client* const client_;
-
- // Task runner to use for queuing client_ callbacks.
- TaskRunner* const task_runner_;
-
OSP_DISALLOW_COPY_AND_ASSIGN(UdpSocket);
};
diff --git a/platform/api/udp_socket_unittest.cc b/platform/api/udp_socket_unittest.cc
deleted file mode 100644
index 25d33c7..0000000
--- a/platform/api/udp_socket_unittest.cc
+++ /dev/null
@@ -1,60 +0,0 @@
-// Copyright 2019 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 "platform/api/udp_socket.h"
-
-#include "gtest/gtest.h"
-#include "platform/api/time.h"
-#include "platform/test/fake_clock.h"
-#include "platform/test/fake_task_runner.h"
-#include "platform/test/fake_udp_socket.h"
-
-namespace openscreen {
-namespace platform {
-namespace {
-
-class MockCallbacks : public UdpSocket::LifetimeObserver {
- public:
- MOCK_METHOD1(OnCreate, void(UdpSocket*));
- MOCK_METHOD1(OnDestroy, void(UdpSocket*));
-};
-
-} // namespace
-
-using testing::_;
-
-TEST(UdpSocketTest, TestDeletionWithoutCallbackSet) {
- FakeClock clock(Clock::now());
- FakeTaskRunner task_runner(&clock);
- FakeUdpSocket::MockClient client;
- MockCallbacks callbacks;
- EXPECT_CALL(callbacks, OnCreate(_)).Times(0);
- EXPECT_CALL(callbacks, OnDestroy(_)).Times(0);
- {
- UdpSocket* socket =
- new FakeUdpSocket(&task_runner, &client, UdpSocket::Version::kV4);
- delete socket;
- }
-}
-
-TEST(UdpSocketTest, TestCallbackCalledOnDeletion) {
- FakeClock clock(Clock::now());
- FakeTaskRunner task_runner(&clock);
- FakeUdpSocket::MockClient client;
- MockCallbacks callbacks;
- EXPECT_CALL(callbacks, OnCreate(_)).Times(1);
- EXPECT_CALL(callbacks, OnDestroy(_)).Times(1);
- UdpSocket::SetLifetimeObserver(&callbacks);
-
- {
- UdpSocket* socket =
- new FakeUdpSocket(&task_runner, &client, UdpSocket::Version::kV4);
- delete socket;
- }
-
- UdpSocket::SetLifetimeObserver(nullptr);
-}
-
-} // namespace platform
-} // namespace openscreen