dispatcher: Remove obsolete runtime feature envoy.reloadable_features…

….activate_fds_next_event_loop (envoyproxy#14055)

Feature flag was introduced in PR envoyproxy#11750 on 2020-06-27. Would be good to obsolete in preparation to changes to activate/setEnable behavior which should make activate behavior more intuitive and simplify edge trigger emulation and userspace file event implementations.

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Qin Qin <qqin@google.com>

docs/root/version_history/current.rst

@@ -43,6 +43,7 @@ Removed Config or Runtime
 -------------------------
 *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
 
+* dispatcher: removed legacy socket read/write resumption code path and runtime guard `envoy.reloadable_features.activate_fds_next_event_loop`.
 * ext_authz: removed auto ignore case in HTTP-based `ext_authz` header matching and the runtime guard `envoy.reloadable_features.ext_authz_http_service_enable_case_sensitive_string_matcher`. To ignore case, set the :ref:`ignore_case <envoy_api_field_type.matcher.StringMatcher.ignore_case>` field to true.
 * http: flip default HTTP/1 and HTTP/2 server codec implementations to new codecs that remove the use of exceptions for control flow. To revert to old codec behavior, set the runtime feature `envoy.reloadable_features.new_codec_behavior` to false.
 * http: removed `envoy.reloadable_features.http1_flood_protection` and legacy code path for turning flood protection off.

include/envoy/network/listener.h

@@ -378,9 +378,6 @@ class UdpListener : public virtual Listener {
 
   /**
    * Make this listener readable at the beginning of the next event loop.
-   *
-   * @note: it may become readable during the current loop if feature
-   * ``envoy.reloadable_features.activate_fds_next_event_loop`` is disabled.
    */
   virtual void activateRead() PURE;
 };

source/common/event/BUILD

@@ -33,7 +33,6 @@ envoy_cc_library(
         "//source/common/network:dns_lib",
         "//source/common/network:connection_lib",
         "//source/common/network:listener_lib",
-        "//source/common/runtime:runtime_features_lib",
     ] + select({
         "//bazel:apple": ["//source/common/network:apple_dns_lib"],
         "//conditions:default": [],

source/common/event/file_event_impl.cc

@@ -4,7 +4,6 @@
 
 #include "common/common/assert.h"
 #include "common/event/dispatcher_impl.h"
-#include "common/runtime/runtime_features.h"
 
 #include "event2/event.h"
 
@@ -14,16 +13,10 @@ namespace Event {
 FileEventImpl::FileEventImpl(DispatcherImpl& dispatcher, os_fd_t fd, FileReadyCb cb,
                              FileTriggerType trigger, uint32_t events)
     : cb_(cb), fd_(fd), trigger_(trigger),
-      activate_fd_events_next_event_loop_(
-          // Only read the runtime feature if the runtime loader singleton has already been created.
-          // Attempts to access runtime features too early in the initialization sequence triggers
-          // some spurious, scary-looking logs about not being able to read runtime feature config
-          // from the singleton. These warnings are caused by creation of filesystem watchers as
-          // part of the process of loading the runtime configuration from disk.
-          Runtime::LoaderSingleton::getExisting()
-              ? Runtime::runtimeFeatureEnabled(
-                    "envoy.reloadable_features.activate_fds_next_event_loop")
-              : true) {
+      activation_cb_(dispatcher.createSchedulableCallback([this]() {
+        ASSERT(injected_activation_events_ != 0);
+        mergeInjectedEventsAndRunCb(0);
+      })) {
   // Treat the lack of a valid fd (which in practice should only happen if we run out of FDs) as
   // an OOM condition and just crash.
   RELEASE_ASSERT(SOCKET_VALID(fd), "");
@@ -33,12 +26,6 @@ FileEventImpl::FileEventImpl(DispatcherImpl& dispatcher, os_fd_t fd, FileReadyCb
 #endif
   assignEvents(events, &dispatcher.base());
   event_add(&raw_event_, nullptr);
-  if (activate_fd_events_next_event_loop_) {
-    activation_cb_ = dispatcher.createSchedulableCallback([this]() {
-      ASSERT(injected_activation_events_ != 0);
-      mergeInjectedEventsAndRunCb(0);
-    });
-  }
 }
 
 void FileEventImpl::activate(uint32_t events) {
@@ -47,26 +34,6 @@ void FileEventImpl::activate(uint32_t events) {
   // Only supported event types are set.
   ASSERT((events & (FileReadyType::Read | FileReadyType::Write | FileReadyType::Closed)) == events);
 
-  if (!activate_fd_events_next_event_loop_) {
-    // Legacy implementation
-    int libevent_events = 0;
-    if (events & FileReadyType::Read) {
-      libevent_events |= EV_READ;
-    }
-
-    if (events & FileReadyType::Write) {
-      libevent_events |= EV_WRITE;
-    }
-
-    if (events & FileReadyType::Closed) {
-      libevent_events |= EV_CLOSED;
-    }
-
-    ASSERT(libevent_events);
-    event_active(&raw_event_, libevent_events, 0);
-    return;
-  }
-
   // Schedule the activation callback so it runs as part of the next loop iteration if it is not
   // already scheduled.
   if (injected_activation_events_ == 0) {
@@ -109,7 +76,7 @@ void FileEventImpl::assignEvents(uint32_t events, event_base* base) {
 }
 
 void FileEventImpl::setEnabled(uint32_t events) {
-  if (activate_fd_events_next_event_loop_ && injected_activation_events_ != 0) {
+  if (injected_activation_events_ != 0) {
     // Clear pending events on updates to the fd event mask to avoid delivering events that are no
     // longer relevant. Updating the event mask will reset the fd edge trigger state so the proxy
     // will be able to determine the fd read/write state without need for the injected activation
@@ -125,7 +92,7 @@ void FileEventImpl::setEnabled(uint32_t events) {
 }
 
 void FileEventImpl::mergeInjectedEventsAndRunCb(uint32_t events) {
-  if (activate_fd_events_next_event_loop_ && injected_activation_events_ != 0) {
+  if (injected_activation_events_ != 0) {
     events |= injected_activation_events_;
     injected_activation_events_ = 0;
     activation_cb_->cancel();

source/common/event/file_event_impl.h

@@ -36,10 +36,6 @@ class FileEventImpl : public FileEvent, ImplBase {
   uint32_t injected_activation_events_{};
   // Used to schedule delayed event activation. Armed iff pending_activation_events_ != 0.
   SchedulableCallbackPtr activation_cb_;
-  // Latched "envoy.reloadable_features.activate_fds_next_event_loop" runtime feature. If true, fd
-  // events scheduled via activate are evaluated in the next iteration of the event loop after
-  // polling and activating new fd events.
-  const bool activate_fd_events_next_event_loop_;
 };
 } // namespace Event
 } // namespace Envoy

source/common/event/libevent_scheduler.h

@@ -43,8 +43,6 @@ namespace Event {
 // The same mechanism implements both of these operations, so they are invoked as a group.
 // - Event::SchedulableCallback::scheduleCallbackCurrentIteration(). Each of these callbacks is
 // scheduled and invoked independently.
-// - Event::FileEvent::activate() if "envoy.reloadable_features.activate_fds_next_event_loop"
-// runtime feature is disabled.
 // - Event::Timer::enableTimer(0) if "envoy.reloadable_features.activate_timers_next_event_loop"
 // runtime feature is disabled.
 //

source/common/runtime/runtime_features.cc

@@ -57,7 +57,6 @@ constexpr const char* runtime_features[] = {
     // Begin alphabetically sorted section.
     "envoy.deprecated_features.allow_deprecated_extension_names",
     "envoy.reloadable_features.always_apply_route_header_rules",
-    "envoy.reloadable_features.activate_fds_next_event_loop",
     "envoy.reloadable_features.activate_timers_next_event_loop",
     "envoy.reloadable_features.allow_500_after_100",
     "envoy.reloadable_features.allow_prefetch",

test/common/event/file_event_impl_test.cc

@@ -47,34 +47,25 @@ class FileEventImplTest : public testing::Test {
   Api::OsSysCalls& os_sys_calls_;
 };
 
-class FileEventImplActivateTest
-    : public testing::TestWithParam<std::tuple<Network::Address::IpVersion, bool>> {
+class FileEventImplActivateTest : public testing::TestWithParam<Network::Address::IpVersion> {
 public:
-  FileEventImplActivateTest() : os_sys_calls_(Api::OsSysCallsSingleton::get()) {
-    Runtime::LoaderSingleton::getExisting()->mergeValues(
-        {{"envoy.reloadable_features.activate_fds_next_event_loop",
-          activateFdsNextEventLoop() ? "true" : "false"}});
-  }
+  FileEventImplActivateTest() : os_sys_calls_(Api::OsSysCallsSingleton::get()) {}
 
   static void onWatcherReady(evwatch*, const evwatch_prepare_cb_info*, void* arg) {
     // `arg` contains the ReadyWatcher passed in from evwatch_prepare_new.
     auto watcher = static_cast<ReadyWatcher*>(arg);
     watcher->ready();
   }
 
-  int domain() {
-    return std::get<0>(GetParam()) == Network::Address::IpVersion::v4 ? AF_INET : AF_INET6;
-  }
-  bool activateFdsNextEventLoop() { return std::get<1>(GetParam()); }
+  int domain() { return GetParam() == Network::Address::IpVersion::v4 ? AF_INET : AF_INET6; }
 
 protected:
   Api::OsSysCalls& os_sys_calls_;
   TestScopedRuntime scoped_runtime_;
 };
 
-INSTANTIATE_TEST_SUITE_P(
-    IpVersions, FileEventImplActivateTest,
-    testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), testing::Bool()));
+INSTANTIATE_TEST_SUITE_P(IpVersions, FileEventImplActivateTest,
+                         testing::ValuesIn(TestEnvironment::getIpVersionsForTest()));
 
 TEST_P(FileEventImplActivateTest, Activate) {
   os_fd_t fd = os_sys_calls_.socket(domain(), SOCK_STREAM, 0).rc_;
@@ -160,29 +151,17 @@ TEST_P(FileEventImplActivateTest, ActivateChaining) {
   EXPECT_CALL(fd_event, ready());
   EXPECT_CALL(read_event, ready());
   EXPECT_CALL(write_event, ready());
-  if (activateFdsNextEventLoop()) {
-    // Second loop iteration: handle write and close events scheduled while handling read.
-    EXPECT_CALL(prepare_watcher, ready());
-    EXPECT_CALL(fd_event, ready());
-    EXPECT_CALL(write_event, ready());
-    EXPECT_CALL(closed_event, ready());
-    // Third loop iteration: handle close event scheduled while handling write.
-    EXPECT_CALL(prepare_watcher, ready());
-    EXPECT_CALL(fd_event, ready());
-    EXPECT_CALL(closed_event, ready());
-    // Fourth loop iteration: poll returned no new real events.
-    EXPECT_CALL(prepare_watcher, ready());
-  } else {
-    // Same loop iteration activation: handle write and close events scheduled while handling read.
-    EXPECT_CALL(fd_event, ready());
-    EXPECT_CALL(write_event, ready());
-    EXPECT_CALL(closed_event, ready());
-    // Second same loop iteration activation: handle close event scheduled while handling write.
-    EXPECT_CALL(fd_event, ready());
-    EXPECT_CALL(closed_event, ready());
-    // Second loop iteration: poll returned no new real events.
-    EXPECT_CALL(prepare_watcher, ready());
-  }
+  // Second loop iteration: handle write and close events scheduled while handling read.
+  EXPECT_CALL(prepare_watcher, ready());
+  EXPECT_CALL(fd_event, ready());
+  EXPECT_CALL(write_event, ready());
+  EXPECT_CALL(closed_event, ready());
+  // Third loop iteration: handle close event scheduled while handling write.
+  EXPECT_CALL(prepare_watcher, ready());
+  EXPECT_CALL(fd_event, ready());
+  EXPECT_CALL(closed_event, ready());
+  // Fourth loop iteration: poll returned no new real events.
+  EXPECT_CALL(prepare_watcher, ready());
 
   file_event->activate(FileReadyType::Read);
   dispatcher->run(Event::Dispatcher::RunType::NonBlock);