diff --git a/source/common/event/file_event_impl.cc b/source/common/event/file_event_impl.cc index 252bb53650f9e..45f01dffa467c 100644 --- a/source/common/event/file_event_impl.cc +++ b/source/common/event/file_event_impl.cc @@ -85,7 +85,16 @@ void FileEventImpl::assignEvents(uint32_t events, event_base* base) { void FileEventImpl::updateEvents(uint32_t events) { ASSERT(dispatcher_.isThreadSafe()); - if (events == enabled_events_) { + // The update can be skipped in cases where the old and new event mask are the same if the fd is + // using Level or EmulatedEdge trigger modes, but not Edge trigger mode. When the fd is registered + // in edge trigger mode, re-registering the fd will force re-computation of the readable/writable + // state even in cases where the event mask is not changing. See + // https://github.com/envoyproxy/envoy/pull/16389 for more details. + // TODO(antoniovicente) Consider ways to optimize away event registration updates in edge trigger + // mode once setEnabled stops clearing injected_activation_events_ before calling updateEvents + // and/or implement optimizations at the Network::ConnectionImpl level to reduce the number of + // calls to setEnabled. + if (events == enabled_events_ && trigger_ != FileTriggerType::Edge) { return; } auto* base = event_get_base(&raw_event_); diff --git a/test/common/event/file_event_impl_test.cc b/test/common/event/file_event_impl_test.cc index 64abdc349ea9b..1dc8eae1984dd 100644 --- a/test/common/event/file_event_impl_test.cc +++ b/test/common/event/file_event_impl_test.cc @@ -35,6 +35,17 @@ class FileEventImplTest : public testing::Test { ASSERT_EQ(sizeof(data), static_cast(result.rc_)); } + void clearReadable() { + // Read the data from the socket so it is no longer readable. + char buffer[10]; + struct iovec vec { + buffer, sizeof(buffer) + }; + const Api::SysCallSizeResult result = os_sys_calls_.readv(fds_[0], &vec, 1); + EXPECT_LT(0, static_cast(result.rc_)); + EXPECT_GT(sizeof(buffer), static_cast(result.rc_)); + } + void TearDown() override { os_sys_calls_.close(fds_[0]); os_sys_calls_.close(fds_[1]); @@ -253,18 +264,17 @@ TEST_F(FileEventImplTest, EdgeTrigger) { #endif TEST_F(FileEventImplTest, LevelTrigger) { + testing::InSequence s; ReadyWatcher read_event; - EXPECT_CALL(read_event, ready()).Times(2); ReadyWatcher write_event; - EXPECT_CALL(write_event, ready()).Times(2); - int count = 2; + int count = 0; Event::FileEventPtr file_event = dispatcher_->createFileEvent( fds_[0], [&](uint32_t events) -> void { - if (count-- == 0) { + ASSERT(count > 0); + if (--count == 0) { dispatcher_->exit(); - return; } if (events & FileReadyType::Read) { read_event.ready(); @@ -276,14 +286,43 @@ TEST_F(FileEventImplTest, LevelTrigger) { }, FileTriggerType::Level, FileReadyType::Read | FileReadyType::Write); + // Expect events to be delivered twice since count=2 and level events are delivered on each + // iteration until the fd state changes. + EXPECT_CALL(read_event, ready()); + EXPECT_CALL(write_event, ready()); + EXPECT_CALL(read_event, ready()); + EXPECT_CALL(write_event, ready()); + count = 2; + dispatcher_->run(Event::Dispatcher::RunType::Block); + + // Change the event mask to just Write and verify that only that event is delivered. + EXPECT_CALL(read_event, ready()).Times(0); + EXPECT_CALL(write_event, ready()); + file_event->setEnabled(FileReadyType::Write); + count = 1; + dispatcher_->run(Event::Dispatcher::RunType::Block); + + // Activate read, and verify it is delivered despite not being part of the enabled event mask. + EXPECT_CALL(read_event, ready()); + EXPECT_CALL(write_event, ready()); + file_event->activate(FileReadyType::Read); + count = 1; + dispatcher_->run(Event::Dispatcher::RunType::Block); + + // Activate read and then call setEnabled. Verify that the read event is not delivered; setEnabled + // clears events from explicit calls to activate. + EXPECT_CALL(read_event, ready()).Times(0); + EXPECT_CALL(write_event, ready()); + file_event->activate(FileReadyType::Read); + file_event->setEnabled(FileReadyType::Write); + count = 1; dispatcher_->run(Event::Dispatcher::RunType::Block); } TEST_F(FileEventImplTest, SetEnabled) { + testing::InSequence s; ReadyWatcher read_event; - EXPECT_CALL(read_event, ready()).Times(2); ReadyWatcher write_event; - EXPECT_CALL(write_event, ready()).Times(2); const FileTriggerType trigger = Event::PlatformDefaultTriggerType; @@ -300,17 +339,125 @@ TEST_F(FileEventImplTest, SetEnabled) { }, trigger, FileReadyType::Read | FileReadyType::Write); + EXPECT_CALL(read_event, ready()); file_event->setEnabled(FileReadyType::Read); dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + EXPECT_CALL(write_event, ready()); file_event->setEnabled(FileReadyType::Write); dispatcher_->run(Event::Dispatcher::RunType::NonBlock); file_event->setEnabled(0); dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + EXPECT_CALL(read_event, ready()); + EXPECT_CALL(write_event, ready()); + file_event->setEnabled(FileReadyType::Read | FileReadyType::Write); + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + + // Run a manual event to ensure that previous expectations are satisfied before moving on. + ReadyWatcher manual_event; + EXPECT_CALL(manual_event, ready()); + manual_event.ready(); + + clearReadable(); + + file_event->setEnabled(FileReadyType::Read); + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + + EXPECT_CALL(write_event, ready()); + file_event->setEnabled(FileReadyType::Write); + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + + EXPECT_CALL(write_event, ready()); + file_event->setEnabled(FileReadyType::Read | FileReadyType::Write); + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + + // Repeat the previous registration, verify that write event is delivered again. + EXPECT_CALL(write_event, ready()); file_event->setEnabled(FileReadyType::Read | FileReadyType::Write); dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + + // Synthetic read events are delivered even if the active registration doesn't contain them. + EXPECT_CALL(read_event, ready()); + file_event->activate(FileReadyType::Read); + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + + // Run a manual event to ensure that previous expectations are satisfied before moving on. + EXPECT_CALL(manual_event, ready()); + manual_event.ready(); + + // Do a read activation followed setEnabled to verify that the activation is cleared. + EXPECT_CALL(write_event, ready()); + file_event->activate(FileReadyType::Read); + file_event->setEnabled(FileReadyType::Read | FileReadyType::Write); + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + + // Repeat the previous steps but with the same input to setEnabled to verify that the activation + // is cleared even in cases where the setEnable mask hasn't changed. + EXPECT_CALL(write_event, ready()); + file_event->activate(FileReadyType::Read); + file_event->setEnabled(FileReadyType::Read | FileReadyType::Write); + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); +} + +TEST_F(FileEventImplTest, RegisterIfEmulatedEdge) { + // Test only applies if using EmulatedEdge trigger mode + if constexpr (PlatformDefaultTriggerType != FileTriggerType::EmulatedEdge) { + return; + } + + testing::InSequence s; + ReadyWatcher read_event; + ReadyWatcher write_event; + + const FileTriggerType trigger = Event::PlatformDefaultTriggerType; + + Event::FileEventPtr file_event = dispatcher_->createFileEvent( + fds_[0], + [&](uint32_t events) -> void { + if (events & FileReadyType::Read) { + read_event.ready(); + } + + if (events & FileReadyType::Write) { + write_event.ready(); + } + }, + trigger, FileReadyType::Read | FileReadyType::Write); + + EXPECT_CALL(read_event, ready()).Times(0); + EXPECT_CALL(write_event, ready()).Times(0); + file_event->unregisterEventIfEmulatedEdge(FileReadyType::Read | FileReadyType::Write); + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + + EXPECT_CALL(read_event, ready()); + file_event->registerEventIfEmulatedEdge(FileReadyType::Read); + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + + EXPECT_CALL(write_event, ready()); + file_event->registerEventIfEmulatedEdge(FileReadyType::Write); + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + + EXPECT_CALL(read_event, ready()); + file_event->registerEventIfEmulatedEdge(FileReadyType::Read | FileReadyType::Write); + file_event->unregisterEventIfEmulatedEdge(FileReadyType::Write); + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + + EXPECT_CALL(read_event, ready()).Times(0); + EXPECT_CALL(write_event, ready()).Times(0); + file_event->unregisterEventIfEmulatedEdge(FileReadyType::Read); + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + + EXPECT_CALL(read_event, ready()); + EXPECT_CALL(write_event, ready()); + file_event->registerEventIfEmulatedEdge(FileReadyType::Read | FileReadyType::Write); + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + + // Events are delivered once due to auto unregistration after they are delivered. + EXPECT_CALL(read_event, ready()).Times(0); + EXPECT_CALL(write_event, ready()).Times(0); + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); } } // namespace