Skip to content
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

network: delegate file event creation to io handle #12720

Merged
merged 2 commits into from
Aug 20, 2020
Merged

network: delegate file event creation to io handle #12720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8163189
network: delegate file event creation to io handle
florincoras Aug 18, 2020
abcaf98
Merge branch 'master' into iohandle_event
florincoras Aug 19, 2020
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
1 change: 1 addition & 0 deletions include/envoy/network/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ envoy_cc_library(
":address_interface",
"//include/envoy/api:io_error_interface",
"//include/envoy/api:os_sys_calls_interface",
"//include/envoy/event:file_event_interface",
"//source/common/common:assert_lib",
],
)
Expand Down
17 changes: 17 additions & 0 deletions include/envoy/network/io_handle.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "envoy/api/io_error.h"
#include "envoy/common/platform.h"
#include "envoy/common/pure.h"
#include "envoy/event/file_event.h"
#include "envoy/network/address.h"

#include "absl/container/fixed_array.h"
Expand All @@ -15,6 +16,10 @@ namespace Buffer {
struct RawSlice;
} // namespace Buffer

namespace Event {
class Dispatcher;
} // namespace Event

using RawSliceArrays = absl::FixedArray<absl::FixedArray<Buffer::RawSlice>>;

namespace Network {
Expand Down Expand Up @@ -223,6 +228,18 @@ class IoHandle {
* @return peer's address as @ref Address::InstanceConstSharedPtr
*/
virtual Address::InstanceConstSharedPtr peerAddress() PURE;

/**
* Creates a file event that will signal when the io handle is readable, writable or closed.
* @param dispatcher dispatcher to be used to allocate the file event.
* @param cb supplies the callback to fire when the handle is ready.
* @param trigger specifies whether to edge or level trigger.
* @param events supplies a logical OR of @ref Event::FileReadyType events that the file event
* should initially listen on.
* @return @ref Event::FileEventPtr
*/
virtual Event::FileEventPtr createFileEvent(Event::Dispatcher& dispatcher, Event::FileReadyCb cb,
Copy link
Member

Choose a reason for hiding this comment

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

The long term plan continues to be to completely remove "fd" from all top level dispatcher interfaces, right? My main concern is there is nothing right now that prevents not calling the original FileEvent factory function with the IoHandle fd, right? Can we add relevant comments around usage, TODOs for eventual removal, warnings, etc.? My quick take would be that if we have a similar function on the filesystem interface we might be able to completely remove the public interface that allows file event creation from fd sooner rather than later (I'm not sure what else actually uses that factory currently)? wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's exactly right. The goal is to use IoHandles instead of fds whenever possible.

Apart from the sockets, others that use the dispatcher's original FileEvent factory function are:

  • WatcherImpl, which creates inotify events using raw fds
  • DnsImpl, which relies on raw fds from Ares
  • HotRestartingParent, which also uses raw fds

In theory, we can change all of these, including IoSocketHandleImpl to allocate FileEventPtrs directly and then completely remove the dispatcher's public interface. The only thing we'll miss out on is the isThreadSafe() check, but maybe we can make the function public? I can take care of this but should we do it as part of this PR or a separate one?

Finally, out of those 3, maybe we should update at least DnsImpl to use IoHandles (not sure if that's easy).

Copy link
Member

Choose a reason for hiding this comment

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

In theory, we can change all of these, including IoSocketHandleImpl to allocate FileEventPtrs directly and then completely remove the dispatcher's public interface. The only thing we'll miss out on is the isThreadSafe() check, but maybe we can make the function public? I can take care of this but should we do it as part of this PR or a separate one?

I would prefer we just do it now if you are up for it, but if you feel strongly we can do it as a fast follow up?

Finally, out of those 3, maybe we should update at least DnsImpl to use IoHandles (not sure if that's easy).

Yeah that would be nice. Maybe add a TODO there for follow up later?

Copy link
Member Author

@florincoras florincoras Aug 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Was about to start work on it but it turns out FileEventImpl depends on DispatcherImpl and all is bundled in //source/common/event:dispatcher_lib. Trying to add the latter as dependency to the default_socket_interface_lib quickly runs into circular dependencies. I guess this is because we don't want to expose the scheduler's implementation, i.e., libevent's event_base, in the dispatcher's interface.

Unless there's a smarter way, we'll have have to separate FileEventImpl from DispatcherImpl and that might not be trivial (they'd have to exchange libevent specific objects over generic apis). The alternative would be to still use the dispatcher as a FileEvent factory and work towards removing the fd from IoHandle (more of that here and #12736). But that exercise is also not trivial because "higher level transports" like tls and quic do want to see the raw fds. So it might be some time until we cleanup everything.

What option would you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Eliminating use of the IoHandle::fd() method seems like progress.

Direct creation of FileEvents instead of going through the dispatcher would reduce out ability to mock out FileEvent operations or inject test versions that can easily be marked read/write blocked, unless we add some factory interface similar to the one provided by the dispatchers today.

Copy link
Member

Choose a reason for hiding this comment

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

Yes potentially we can just have a format/lint checker while we do further cleanups.

Copy link
Member

Choose a reason for hiding this comment

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

and yes maybe also rename fdDoNotUse() or something until we fix it all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Works for me! Can we do it after all the PRs are merged to limit impact, i.e., change name and then delete them?

Copy link
Member

Choose a reason for hiding this comment

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

Yup that's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Added to my todo list :-)

Event::FileTriggerType trigger, uint32_t events) PURE;
};

using IoHandlePtr = std::unique_ptr<IoHandle>;
Expand Down
1 change: 1 addition & 0 deletions source/common/network/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ envoy_cc_library(
":io_socket_error_lib",
":socket_interface_lib",
":socket_lib",
"//include/envoy/event:dispatcher_interface",
"//include/envoy/network:io_handle_interface",
"//source/common/api:os_sys_calls_lib",
"@envoy_api//envoy/extensions/network/socket_interface/v3:pkg_cc_proto",
Expand Down
6 changes: 3 additions & 3 deletions source/common/network/connection_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,9 @@ ConnectionImpl::ConnectionImpl(Event::Dispatcher& dispatcher, ConnectionSocketPt

// We never ask for both early close and read at the same time. If we are reading, we want to
// consume all available data.
file_event_ = dispatcher_.createFileEvent(
ConnectionImpl::ioHandle().fd(), [this](uint32_t events) -> void { onFileEvent(events); },
trigger, Event::FileReadyType::Read | Event::FileReadyType::Write);
file_event_ = socket_->ioHandle().createFileEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The RELEASE_ASSERT in line 62 above may make more sense in IoSocketHandleImpl::createFileEvent, DispatcherImpl::createFileEvent or FileEventImpl so we can remove the call to the ioHandle fd method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was actually planning a separate PR that removes all of those asserts! Should I do this one in particular here or is it fine if we postpone it to the next PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Next PR is fine.

dispatcher_, [this](uint32_t events) -> void { onFileEvent(events); }, trigger,
Event::FileReadyType::Read | Event::FileReadyType::Write);

transport_socket_->setTransportSocketCallbacks(*this);
}
Expand Down
7 changes: 7 additions & 0 deletions source/common/network/io_socket_handle_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,5 +472,12 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() {
return Address::addressFromSockAddr(ss, ss_len);
}

Event::FileEventPtr IoSocketHandleImpl::createFileEvent(Event::Dispatcher& dispatcher,
Event::FileReadyCb cb,
Event::FileTriggerType trigger,
uint32_t events) {
return dispatcher.createFileEvent(fd_, cb, trigger, events);
}

} // namespace Network
} // namespace Envoy
3 changes: 3 additions & 0 deletions source/common/network/io_socket_handle_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "envoy/api/io_error.h"
#include "envoy/api/os_sys_calls.h"
#include "envoy/common/platform.h"
#include "envoy/event/dispatcher.h"
#include "envoy/network/io_handle.h"

#include "common/common/logger.h"
Expand Down Expand Up @@ -58,6 +59,8 @@ class IoSocketHandleImpl : public IoHandle, protected Logger::Loggable<Logger::I
absl::optional<int> domain() override;
Address::InstanceConstSharedPtr localAddress() override;
Address::InstanceConstSharedPtr peerAddress() override;
Event::FileEventPtr createFileEvent(Event::Dispatcher& dispatcher, Event::FileReadyCb cb,
Event::FileTriggerType trigger, uint32_t events) override;

protected:
// Converts a SysCallSizeResult to IoCallUint64Result.
Expand Down
4 changes: 2 additions & 2 deletions source/common/network/listener_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ void ListenerImpl::setupServerSocket(Event::DispatcherImpl& dispatcher, Socket&

// Although onSocketEvent drains to completion, use level triggered mode to avoid potential
// loss of the trigger due to transient accept errors.
file_event_ = dispatcher.createFileEvent(
socket.ioHandle().fd(), [this](uint32_t events) -> void { onSocketEvent(events); },
file_event_ = socket.ioHandle().createFileEvent(
dispatcher, [this](uint32_t events) -> void { onSocketEvent(events); },
Event::FileTriggerType::Level, Event::FileReadyType::Read);

if (!Network::Socket::applyOptions(socket.options(), socket,
Expand Down
4 changes: 2 additions & 2 deletions source/common/network/udp_listener_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ namespace Network {
UdpListenerImpl::UdpListenerImpl(Event::DispatcherImpl& dispatcher, SocketSharedPtr socket,
UdpListenerCallbacks& cb, TimeSource& time_source)
: BaseListenerImpl(dispatcher, std::move(socket)), cb_(cb), time_source_(time_source) {
file_event_ = dispatcher_.createFileEvent(
socket_->ioHandle().fd(), [this](uint32_t events) -> void { onSocketEvent(events); },
file_event_ = socket_->ioHandle().createFileEvent(
dispatcher, [this](uint32_t events) -> void { onSocketEvent(events); },
Event::PlatformDefaultTriggerType, Event::FileReadyType::Read | Event::FileReadyType::Write);

ASSERT(file_event_);
Expand Down
4 changes: 2 additions & 2 deletions source/extensions/filters/listener/http_inspector/http_inspector.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ Network::FilterStatus Filter::onAccept(Network::ListenerFilterCallbacks& cb) {
case ParseState::Continue:
// do nothing but create the event
ASSERT(file_event_ == nullptr);
file_event_ = cb.dispatcher().createFileEvent(
socket.ioHandle().fd(),
file_event_ = cb.socket().ioHandle().createFileEvent(
cb.dispatcher(),
[this](uint32_t events) {
ENVOY_LOG(trace, "http inspector event: {}", events);
// inspector is always peeking and can never determine EOF.
Expand Down
4 changes: 2 additions & 2 deletions source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ Network::FilterStatus Filter::onAccept(Network::ListenerFilterCallbacks& cb) {
ENVOY_LOG(debug, "proxy_protocol: New connection accepted");
Network::ConnectionSocket& socket = cb.socket();
ASSERT(file_event_.get() == nullptr);
file_event_ = cb.dispatcher().createFileEvent(
socket.ioHandle().fd(),
file_event_ = socket.ioHandle().createFileEvent(
cb.dispatcher(),
[this](uint32_t events) {
ASSERT(events == Event::FileReadyType::Read);
onRead();
Expand Down
4 changes: 2 additions & 2 deletions source/extensions/filters/listener/tls_inspector/tls_inspector.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ Network::FilterStatus Filter::onAccept(Network::ListenerFilterCallbacks& cb) {
return Network::FilterStatus::Continue;
case ParseState::Continue:
// do nothing but create the event
file_event_ = cb.dispatcher().createFileEvent(
socket.ioHandle().fd(),
file_event_ = socket.ioHandle().createFileEvent(
cb.dispatcher(),
[this](uint32_t events) {
if (events & Event::FileReadyType::Closed) {
config_->stats().connection_closed_.inc();
Expand Down
5 changes: 3 additions & 2 deletions source/extensions/filters/udp/udp_proxy/udp_proxy_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,9 @@ UdpProxyFilter::ActiveSession::ActiveSession(ClusterInfo& cluster,
// NOTE: The socket call can only fail due to memory/fd exhaustion. No local ephemeral port
// is bound until the first packet is sent to the upstream host.
io_handle_(cluster.filter_.createIoHandle(host)),
socket_event_(cluster.filter_.read_callbacks_->udpListener().dispatcher().createFileEvent(
io_handle_->fd(), [this](uint32_t) { onReadReady(); }, Event::PlatformDefaultTriggerType,
socket_event_(io_handle_->createFileEvent(
cluster.filter_.read_callbacks_->udpListener().dispatcher(),
[this](uint32_t) { onReadReady(); }, Event::PlatformDefaultTriggerType,
Event::FileReadyType::Read)) {
ENVOY_LOG(debug, "creating new session: downstream={} local={} upstream={}",
addresses_.peer_->asStringView(), addresses_.local_->asStringView(),
Expand Down
7 changes: 3 additions & 4 deletions source/extensions/quic_listeners/quiche/envoy_quic_client_connection.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,9 @@ uint64_t EnvoyQuicClientConnection::maxPacketSize() const {

void EnvoyQuicClientConnection::setUpConnectionSocket() {
if (connectionSocket()->ioHandle().isOpen()) {
file_event_ = dispatcher_.createFileEvent(
connectionSocket()->ioHandle().fd(),
[this](uint32_t events) -> void { onFileEvent(events); }, Event::FileTriggerType::Edge,
Event::FileReadyType::Read | Event::FileReadyType::Write);
file_event_ = connectionSocket()->ioHandle().createFileEvent(
dispatcher_, [this](uint32_t events) -> void { onFileEvent(events); },
Event::FileTriggerType::Edge, Event::FileReadyType::Read | Event::FileReadyType::Write);

if (!Network::Socket::applyOptions(connectionSocket()->options(), *connectionSocket(),
envoy::config::core::v3::SocketOption::STATE_LISTENING)) {
Expand Down
4 changes: 4 additions & 0 deletions source/extensions/quic_listeners/quiche/quic_io_handle_wrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ class QuicIoHandleWrapper : public Network::IoHandle {
Network::Address::InstanceConstSharedPtr peerAddress() override {
return io_handle_.peerAddress();
}
Event::FileEventPtr createFileEvent(Event::Dispatcher& dispatcher, Event::FileReadyCb cb,
Event::FileTriggerType trigger, uint32_t events) override {
return io_handle_.createFileEvent(dispatcher, cb, trigger, events);
}

private:
Network::IoHandle& io_handle_;
Expand Down
2 changes: 2 additions & 0 deletions test/extensions/filters/listener/http_inspector/http_inspector_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class HttpInspectorTest : public testing::Test {
EXPECT_CALL(socket_, detectedTransportProtocol()).WillRepeatedly(Return("raw_buffer"));
EXPECT_CALL(cb_, dispatcher()).WillRepeatedly(ReturnRef(dispatcher_));
EXPECT_CALL(testing::Const(socket_), ioHandle()).WillRepeatedly(ReturnRef(*io_handle_));
EXPECT_CALL(socket_, ioHandle()).WillRepeatedly(ReturnRef(*io_handle_));

if (include_inline_recv) {
EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK))
Expand Down Expand Up @@ -72,6 +73,7 @@ TEST_F(HttpInspectorTest, SkipHttpInspectForTLS) {
filter_ = std::make_unique<Filter>(cfg_);

EXPECT_CALL(cb_, socket()).WillRepeatedly(ReturnRef(socket_));
EXPECT_CALL(socket_, ioHandle()).WillRepeatedly(ReturnRef(*io_handle_));
EXPECT_CALL(socket_, detectedTransportProtocol()).WillRepeatedly(Return("TLS"));
EXPECT_EQ(filter_->onAccept(cb_), Network::FilterStatus::Continue);
}
Expand Down
6 changes: 2 additions & 4 deletions test/extensions/filters/udp/udp_proxy/udp_proxy_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,8 @@ class UdpProxyFilterTest : public testing::Test {
new_session.idle_timer_ = new Event::MockTimer(&callbacks_.udp_listener_.dispatcher_);
EXPECT_CALL(*filter_, createIoHandle(_))
.WillOnce(Return(ByMove(Network::IoHandlePtr{test_sessions_.back().io_handle_})));
EXPECT_CALL(*new_session.io_handle_, fd());
EXPECT_CALL(
callbacks_.udp_listener_.dispatcher_,
createFileEvent_(_, _, Event::PlatformDefaultTriggerType, Event::FileReadyType::Read))
EXPECT_CALL(*new_session.io_handle_, createFileEvent_(_, _, Event::PlatformDefaultTriggerType,
Event::FileReadyType::Read))
.WillOnce(DoAll(SaveArg<1>(&new_session.file_event_cb_), Return(nullptr)));
// Internal Buffer is Empty, flush will be a no-op
ON_CALL(callbacks_.udp_listener_, flush())
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 @@ -25,6 +25,7 @@ envoy_cc_mock(
srcs = ["io_handle.cc"],
hdrs = ["io_handle.h"],
deps = [
"//include/envoy/event:dispatcher_interface",
"//include/envoy/network:io_handle_interface",
"//source/common/buffer:buffer_lib",
],
Expand Down
9 changes: 9 additions & 0 deletions test/mocks/network/io_handle.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include "envoy/buffer/buffer.h"
#include "envoy/event/dispatcher.h"
#include "envoy/network/io_handle.h"

#include "gmock/gmock.h"
Expand All @@ -13,6 +14,11 @@ class MockIoHandle : public IoHandle {
MockIoHandle();
~MockIoHandle() override;

Event::FileEventPtr createFileEvent(Event::Dispatcher& dispatcher, Event::FileReadyCb cb,
Event::FileTriggerType trigger, uint32_t events) override {
return Event::FileEventPtr{createFileEvent_(dispatcher, cb, trigger, events)};
}

MOCK_METHOD(os_fd_t, fd, (), (const));
MOCK_METHOD(Api::IoCallUint64Result, close, ());
MOCK_METHOD(bool, isOpen, (), (const));
Expand Down Expand Up @@ -42,6 +48,9 @@ class MockIoHandle : public IoHandle {
MOCK_METHOD(absl::optional<int>, domain, ());
MOCK_METHOD(Address::InstanceConstSharedPtr, localAddress, ());
MOCK_METHOD(Address::InstanceConstSharedPtr, peerAddress, ());
MOCK_METHOD(Event::FileEvent*, createFileEvent_,
(Event::Dispatcher & dispatcher, Event::FileReadyCb cb,
Event::FileTriggerType trigger, uint32_t events));
};

} // namespace Network
Expand Down