Skip to content

listener: fix sharedSocket #17090

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 9 commits into from
Jul 7, 2021
Merged

listener: fix sharedSocket #17090

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
759a38b
fix prod
lambdai Jun 21, 2021
f142345
fix test
lambdai Jun 21, 2021
f55e317
format
lambdai Jun 22, 2021
a12492f
add extra io handle check
lambdai Jun 22, 2021
06d434b
Merge remote-tracking branch 'origin/main' into tcpbindtoport
lambdai Jun 24, 2021
14b244a
Merge branch 'main' into tcpbindtoport
lambdai Jul 1, 2021
34e6c6f
unify udp and tcp listener socket constructor
lambdai Jul 2, 2021
df2d945
clean up
lambdai Jul 2, 2021
60cdee7
update comment
lambdai Jul 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion source/common/network/listen_socket_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ template <>
NetworkListenSocket<NetworkSocketTrait<Socket::Type::Datagram>>::NetworkListenSocket(
const Address::InstanceConstSharedPtr& address,
const Network::Socket::OptionsSharedPtr& options, bool bind_to_port)
: ListenSocketImpl(Network::ioHandleForAddr(Socket::Type::Datagram, address), address) {
: ListenSocketImpl(Network::ioHandleForAddr(Socket::Type::Datagram, address), address),
bind_to_port_(bind_to_port) {
setPrebindSocketOptions();
setupSocket(options, bind_to_port);
}
Expand Down
40 changes: 38 additions & 2 deletions source/common/network/listen_socket_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ template <typename T> class NetworkListenSocket : public ListenSocketImpl {
NetworkListenSocket(const Address::InstanceConstSharedPtr& address,
const Network::Socket::OptionsSharedPtr& options, bool bind_to_port)
: ListenSocketImpl(bind_to_port ? Network::ioHandleForAddr(T::type, address) : nullptr,
address) {
address),
bind_to_port_(bind_to_port) {
// Prebind is applied if the socket is bind to port.
if (bind_to_port) {
RELEASE_ASSERT(io_handle_->isOpen(), "");
Expand All @@ -74,12 +75,19 @@ template <typename T> class NetworkListenSocket : public ListenSocketImpl {

NetworkListenSocket(IoHandlePtr&& io_handle, const Address::InstanceConstSharedPtr& address,
const Network::Socket::OptionsSharedPtr& options)
: ListenSocketImpl(std::move(io_handle), address) {
: ListenSocketImpl(std::move(io_handle), address), bind_to_port_(io_handle_ == nullptr) {
Comment thread
lambdai marked this conversation as resolved.
Outdated
setListenSocketOptions(options);
}

Socket::Type socketType() const override { return T::type; }

// This four override is introduced to execute bind_to_port_ check. We can remove the override in
// the future.
IoHandle& ioHandle() override { return ListenSocketImpl::ioHandle(); }
const IoHandle& ioHandle() const override { return ListenSocketImpl::ioHandle(); }
void close() override { return ListenSocketImpl::close(); }
bool isOpen() const override { return ListenSocketImpl::isOpen(); }

protected:
void setPrebindSocketOptions() {
// On Windows, SO_REUSEADDR does not restrict subsequent bind calls when there is a listener as
Expand All @@ -90,6 +98,8 @@ template <typename T> class NetworkListenSocket : public ListenSocketImpl {
RELEASE_ASSERT(status.rc_ != -1, "failed to set SO_REUSEADDR socket option");
#endif
}
// This flag is used to confirm whether this socket own an io handle.
const bool bind_to_port_;
};

template <>
Expand All @@ -102,6 +112,32 @@ NetworkListenSocket<NetworkSocketTrait<Socket::Type::Datagram>>::NetworkListenSo
const Address::InstanceConstSharedPtr& address,
const Network::Socket::OptionsSharedPtr& options, bool bind_to_port);

template <>
inline IoHandle& NetworkListenSocket<NetworkSocketTrait<Socket::Type::Stream>>::ioHandle() {
ASSERT(bind_to_port_);
return *io_handle_;
}
template <>
inline const IoHandle&
NetworkListenSocket<NetworkSocketTrait<Socket::Type::Stream>>::ioHandle() const {
ASSERT(bind_to_port_);
return *io_handle_;
}

template <> inline void NetworkListenSocket<NetworkSocketTrait<Socket::Type::Stream>>::close() {
if (bind_to_port_) {
if (io_handle_->isOpen()) {
io_handle_->close();
}
}
}

template <>
inline bool NetworkListenSocket<NetworkSocketTrait<Socket::Type::Stream>>::isOpen() const {
ASSERT(bind_to_port_);
return io_handle_->isOpen();
}

template class NetworkListenSocket<NetworkSocketTrait<Socket::Type::Stream>>;
template class NetworkListenSocket<NetworkSocketTrait<Socket::Type::Datagram>>;

Expand Down
7 changes: 6 additions & 1 deletion source/server/listener_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,16 @@ class ListenSocketFactoryImpl : public Network::ListenSocketFactory,
* @return the socket shared by worker threads; otherwise return null.
*/
Network::SocketOptRef sharedSocket() const override {
// If a tcp listener socket doesn't bind to port, there is no listen socket so there is no
// shared socket.
if (socketType() == Network::Socket::Type::Stream && !bind_to_port_) {

@antoniovicente antoniovicente Jun 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this apply only to Stream sockets or should it also apply to other socket types?

@lambdai lambdai Jun 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question!
I think the sharedSocket() matters only if my imagined bind_to_port is on.
It seems udp listener's bind_to_port has a different semantic
@danzh2010 Could you add some comment on the bind_to_port usage in udp/quic listener?

@lambdai lambdai Jun 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless the semantic of bind_to_port in UDP world, the implementation of UDP on bind_to_port = false is that a socket is created so sharedSocket() must returns it.

@lambdai lambdai Jun 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the answer to the original question:
As per the current impl of udp listener, sharedSocket() should return it as in this PR.

However, if udp listener turns to not create socket on bind_to_port==false, this sharedSocket() need to align with that change.

@antoniovicente antoniovicente Jun 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to have the behaviors in sync. bind_to_port == false doesn't make much sense to me for UDP, I wonder if it should be disallowed.

@lambdai lambdai Jun 24, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being said, I'm wondering if returning nullopt in sharedSocket() based on bind_to_port is the right approach? According to the contract sharedSocket() returns nullopt when the sockets among worker threads are not shared, so it is called in ListenerManagerImpl::drainListener() to decide whether to close shared socket or not. This PR seems to change that contract. Should we also change the corresponding logic of closing socket?

I agree this PR is somewhat changing the contract of sharedSocket(). Essentially we have two approaches, one is to provide a dedicate socket impl for listener doesn't bind to port. That specialized socket can be returned by sharedSocket(). The other approach in this PR is not to allow sharedSocket() from the beginning. Close is not an issue as per implementation: no underlying BSD socket is created so we are not leaking fd.

@mattklein123 mattklein123 Jul 2, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned elsewhere, I'm currently working on simplifying all of this code, so I would prefer to not make major changes here which I'm going to undo anyway. Per my above comment, why can't we just check if io_handle_ is null in various places?

@lambdai lambdai Jul 2, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to not make major changes here which I'm going to undo anyway.

I feel this PR has two part: sharedSocket() and the the ListenerSocketImpl.

Your changes will delete sharedSocket(). However, the latter ListenerSocketImpl is needed. The reason is that your commits seems create number of concurrency listen socket regardless it's bind_to_port, for simplicity. I don't know how is that listenersocket closed correctly... I could be wrong though.

@mattklein123 mattklein123 Jul 2, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your changes will delete sharedSocket(). However, the latter ListenerSocketImpl is needed. The reason is that your commits seems create number of concurrency listen socket regardless it's bind_to_port, for simplicity. I don't know how is that listenersocket closed correctly... I could be wrong though.

Sorry I can't parse this. We can review my other change when I post it.

@lambdai lambdai Jul 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, i had the wrong impression of udp listen socket always need a not-nullptr io_handle even when bind_to_port is false. I discovered this "fact" and I now incline to believe the fact is b/c a test case is improperly using listener socket.

return absl::nullopt;
}
if (!reuse_port_) {
ASSERT(socket_ != nullptr);
return *socket_;
}
// If reuse_port is true, always return null, even socket_ is created for reserving
// If reuse_port is true, always return nullopt, even socket_ is created for reserving
// port number.
return absl::nullopt;
}
Expand Down
19 changes: 4 additions & 15 deletions test/common/network/listen_socket_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include "source/common/api/os_sys_calls_impl.h"
#include "source/common/network/io_socket_handle_impl.h"
#include "source/common/network/listen_socket_impl.h"
#include "source/common/network/socket_interface_impl.h"
#include "source/common/network/utility.h"

#include "test/mocks/network/mocks.h"
Expand All @@ -25,18 +24,6 @@ namespace Envoy {
namespace Network {
namespace {

class MockSingleFamilySocketInterface : public SocketInterfaceImpl {
public:
explicit MockSingleFamilySocketInterface(Address::IpVersion version) : version_(version) {}
MOCK_METHOD(IoHandlePtr, socket, (Socket::Type, Address::Type, Address::IpVersion, bool),
(const));
MOCK_METHOD(IoHandlePtr, socket, (Socket::Type, const Address::InstanceConstSharedPtr), (const));
bool ipFamilySupported(int domain) override {
return (version_ == Address::IpVersion::v4) ? domain == AF_INET : domain == AF_INET6;
}
const Address::IpVersion version_;
};

TEST(ConnectionSocketImplTest, LowerCaseRequestedServerName) {
absl::string_view serverName("www.EXAMPLE.com");
absl::string_view expectedServerName("www.example.com");
Expand Down Expand Up @@ -198,7 +185,8 @@ TEST_P(ListenSocketImplTestTcp, CheckIpVersionWithNullLocalAddress) {
}

TEST_P(ListenSocketImplTestTcp, SupportedIpFamilyVirtualSocketIsCreatedWithNoBsdSocketCreated) {
auto mock_interface = std::make_unique<MockSingleFamilySocketInterface>(version_);
auto mock_interface =
std::make_unique<MockSocketInterface>(std::vector<Network::Address::IpVersion>{version_});
auto* mock_interface_ptr = mock_interface.get();
auto any_address = version_ == Address::IpVersion::v4 ? Utility::getIpv4AnyAddress()
: Utility::getIpv6AnyAddress();
Expand All @@ -214,7 +202,8 @@ TEST_P(ListenSocketImplTestTcp, SupportedIpFamilyVirtualSocketIsCreatedWithNoBsd
}

TEST_P(ListenSocketImplTestTcp, DeathAtUnSupportedIpFamilyListenSocket) {
auto mock_interface = std::make_unique<MockSingleFamilySocketInterface>(version_);
auto mock_interface =
std::make_unique<MockSocketInterface>(std::vector<Network::Address::IpVersion>{version_});
auto* mock_interface_ptr = mock_interface.get();
auto the_other_address = version_ == Address::IpVersion::v4 ? Utility::getIpv6AnyAddress()
: Utility::getIpv4AnyAddress();
Expand Down
1 change: 1 addition & 0 deletions test/mocks/network/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ envoy_cc_mock(
"//envoy/network:transport_socket_interface",
"//envoy/server:listener_manager_interface",
"//source/common/network:address_lib",
"//source/common/network:socket_interface_lib",
"//source/common/network:utility_lib",
"//source/common/stats:isolated_store_lib",
"//test/mocks/event:event_mocks",
Expand Down
17 changes: 17 additions & 0 deletions test/mocks/network/mocks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#pragma once

#include <algorithm>
#include <cstdint>
#include <list>
#include <ostream>
Expand All @@ -19,6 +20,7 @@

#include "source/common/network/filter_manager_impl.h"
#include "source/common/network/socket_interface.h"
#include "source/common/network/socket_interface_impl.h"
#include "source/common/stats/isolated_store_impl.h"

#include "test/mocks/event/mocks.h"
Expand Down Expand Up @@ -600,5 +602,20 @@ class MockUdpPacketProcessor : public UdpPacketProcessor {
MOCK_METHOD(size_t, numPacketsExpectedPerEventLoop, (), (const));
};

class MockSocketInterface : public SocketInterfaceImpl {
public:
explicit MockSocketInterface(const std::vector<Address::IpVersion>& versions)
: versions_(versions.begin(), versions.end()) {}
MOCK_METHOD(IoHandlePtr, socket, (Socket::Type, Address::Type, Address::IpVersion, bool),
(const));
MOCK_METHOD(IoHandlePtr, socket, (Socket::Type, const Address::InstanceConstSharedPtr), (const));
bool ipFamilySupported(int domain) override {
const auto to_version = domain == AF_INET ? Address::IpVersion::v4 : Address::IpVersion::v6;
return std::any_of(versions_.begin(), versions_.end(),
[to_version](auto version) { return to_version == version; });
}
const std::vector<Address::IpVersion> versions_;
};

} // namespace Network
} // namespace Envoy
70 changes: 63 additions & 7 deletions test/server/listener_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "source/common/init/manager_impl.h"
#include "source/common/network/address_impl.h"
#include "source/common/network/io_socket_handle_impl.h"
#include "source/common/network/socket_interface_impl.h"
#include "source/common/network/utility.h"
#include "source/common/protobuf/protobuf.h"
#include "source/extensions/filters/listener/original_dst/original_dst.h"
Expand Down Expand Up @@ -1630,6 +1631,10 @@ name: foo

TEST_F(ListenerManagerImplTest, BindToPortEqualToFalse) {
InSequence s;
auto mock_interface = std::make_unique<Network::MockSocketInterface>(
std::vector<Network::Address::IpVersion>{Network::Address::IpVersion::v4});
StackedScopedInjectableLoader<Network::SocketInterface> new_interface(std::move(mock_interface));

ProdListenerComponentFactory real_listener_factory(server_);
EXPECT_CALL(*worker_, start(_, _));
manager_->startWorkers(guard_dog_, callback_.AsStdFunction());
Expand All @@ -1640,31 +1645,82 @@ name: foo
address: 127.0.0.1
port_value: 1234
bind_to_port: false
reuse_port: false
filter_chains:
- filters: []
)EOF";

auto syscall_result = os_sys_calls_actual_.socket(AF_INET, SOCK_STREAM, 0);
ASSERT_TRUE(SOCKET_VALID(syscall_result.rc_));

ListenerHandle* listener_foo = expectListenerCreate(true, true);
EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, ListenSocketCreationParams(false)))
.WillOnce(Invoke([this, &syscall_result, &real_listener_factory](
.WillOnce(Invoke([this, &real_listener_factory](
const Network::Address::InstanceConstSharedPtr& address,
Network::Socket::Type socket_type,
const Network::Socket::OptionsSharedPtr& options,
const ListenSocketCreationParams& params) -> Network::SocketSharedPtr {
EXPECT_CALL(server_, hotRestart).Times(0);
// When bind_to_port is equal to false, create socket fd directly, and do not get socket
// fd through hot restart.
ON_CALL(os_sys_calls_, socket(AF_INET, _, 0)).WillByDefault(Return(syscall_result));
// When bind_to_port is equal to false, the BSD socket is not created at main thread.
EXPECT_CALL(os_sys_calls_, socket(AF_INET, _, 0)).Times(0);
return real_listener_factory.createListenSocket(address, socket_type, options, params);
}));
EXPECT_CALL(listener_foo->target_, initialize());
EXPECT_CALL(*listener_foo, onDestroy());
EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_foo_yaml), "", true));
}

TEST_F(ListenerManagerImplTest, UpdateBindToPortEqualToFalse) {
InSequence s;
auto mock_interface = std::make_unique<Network::MockSocketInterface>(
std::vector<Network::Address::IpVersion>{Network::Address::IpVersion::v4});
StackedScopedInjectableLoader<Network::SocketInterface> new_interface(std::move(mock_interface));

ProdListenerComponentFactory real_listener_factory(server_);
EXPECT_CALL(*worker_, start(_, _));
manager_->startWorkers(guard_dog_, callback_.AsStdFunction());
const std::string listener_foo_yaml = R"EOF(
name: foo
address:
socket_address:
address: 127.0.0.1
port_value: 1234
bind_to_port: false
reuse_port: false
filter_chains:
- filters: []
)EOF";

ListenerHandle* listener_foo = expectListenerCreate(false, true);
EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, ListenSocketCreationParams(false)))
.WillOnce(Invoke([this, &real_listener_factory](
const Network::Address::InstanceConstSharedPtr& address,
Network::Socket::Type socket_type,
const Network::Socket::OptionsSharedPtr& options,
const ListenSocketCreationParams& params) -> Network::SocketSharedPtr {
EXPECT_CALL(server_, hotRestart).Times(0);
// When bind_to_port is equal to false, the BSD socket is not created at main thread.
EXPECT_CALL(os_sys_calls_, socket(AF_INET, _, 0)).Times(0);
return real_listener_factory.createListenSocket(address, socket_type, options, params);
}));
EXPECT_CALL(*worker_, addListener(_, _, _));
EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_foo_yaml), "", true));

worker_->callAddCompletion(true);

EXPECT_CALL(*listener_foo->drain_manager_, drainClose()).WillOnce(Return(false));
EXPECT_CALL(server_.drain_manager_, drainClose()).WillOnce(Return(false));
EXPECT_FALSE(listener_foo->context_->drainDecision().drainClose());

EXPECT_CALL(*worker_, stopListener(_, _));
EXPECT_CALL(*listener_foo->drain_manager_, startDrainSequence(_));

EXPECT_TRUE(manager_->removeListener("foo"));

EXPECT_CALL(*worker_, removeListener(_, _));
listener_foo->drain_manager_->drain_sequence_completion_();

EXPECT_CALL(*listener_foo, onDestroy());
worker_->callRemovalCompletion();
}

TEST_F(ListenerManagerImplTest, DEPRECATED_FEATURE_TEST(DeprecatedBindToPortEqualToFalse)) {
InSequence s;
ProdListenerComponentFactory real_listener_factory(server_);
Expand Down