quic: change QuicNetworkConnectivityObserver interface by danzh2010 · Pull Request #35775 · envoyproxy/envoy

danzh2010 · 2024-08-21T17:01:00Z

Commit Message: change QuicNetworkConnectivityObserver into pure virtual interface for easy mocking and move the implementation into another class QuicNetworkConnectivityObserverImpl. By doing so, envoy_quic_network_observer_registry_factory_lib no longer needs to depend on QUICHE targets and thus no need to provide a dumb implementation if QUICHE is compiled out.

Also change QuicNetworkConnectivityObserver to have 3 interfaces: onNetworkMadeDefault(), onNetworkConnected() and onNetworkDisconnected().

Risk Level: low, new interface not in use
Testing: n/a
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
Runtime guard: N/A

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only · 2024-08-21T17:01:07Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #35775 was opened by danzh2010.

see: more, trace.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2024-08-22T14:11:59Z

/retest

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2024-08-22T19:42:46Z

/retest

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2024-08-22T21:00:05Z

/retest

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2024-08-23T19:54:22Z

/assign @RyanTheOptimist

danzh2010 · 2024-08-23T20:05:54Z

/retest

danzh2010 · 2024-08-23T22:13:31Z

/retest

RyanTheOptimist

Boy, there is a lot going on in this PR. Could we consider breaking it into some smaller pieces?

RyanTheOptimist · 2024-08-23T21:29:25Z

envoy/upstream/cluster_manager.h

+class EnvoyQuicNetworkObserverRegistryFactory;
+class EnvoyQuicNetworkObserverRegistry;
+#else
+// Dumb definitions of QUIC classes if QUICHE is compiled out.


nit: QUICHE is always compiled in, but the QUIC parts may or may not be. So probably s/QUICHE/QUIC/?

RyanTheOptimist · 2024-08-26T15:20:05Z

mobile/library/common/network/connectivity_manager.h

      dns_callbacks_handle_{nullptr};
  Upstream::ClusterManager& cluster_manager_;
+#ifdef ENVOY_ENABLE_QUIC
+  std::unique_ptr<Quic::EnvoyMobileQuicNetworkObserverRegistryFactory>


nit: I looks like this is always created (when QUIC is compiled in). If so, consider making this a non-pointer member. That would avoid the need to explicitly construct it in the constructor.

RyanTheOptimist · 2024-08-26T15:20:17Z

mobile/library/common/network/connectivity_manager.h

  DnsCacheManagerSharedPtr dns_cache_manager_;
  ProxySettingsConstSharedPtr proxy_settings_;
  static NetworkState network_state_;
+  const bool quic_upstream_connection_handle_network_change_;


nit: comment, please.

RyanTheOptimist · 2024-08-26T15:20:34Z

mobile/library/common/network/connectivity_manager.h

  virtual Extensions::Common::DynamicForwardProxy::DnsCacheSharedPtr dnsCache() PURE;
+
+  /**
+   * Called when OS changes the preferred network.


nit: "OS" => "the OS"

RyanTheOptimist · 2024-08-26T15:23:16Z

mobile/library/common/network/envoy_mobile_quic_network_observer_registry_factory.cc

+  ASSERT(Runtime::runtimeFeatureEnabled(
+      "envoy.reloadable_features.quic_upstream_connection_handle_network_change"));
+  dispatcher_.post([this]() {
+    // Retain the existing observers in a list and iterate on the list.


Why do we need to do this? Does the list get potentially mutated?

yes, the conn pool may create new connections as a back fill if the connections on this list gets closed.

RyanTheOptimist · 2024-08-26T15:31:29Z

mobile/library/common/network/envoy_mobile_quic_network_observer_registry_factory.cc

+  ENVOY_LOG_MISC(trace, "Default network changed.");
+  ASSERT(Runtime::runtimeFeatureEnabled(
+      "envoy.reloadable_features.quic_upstream_connection_handle_network_change"));
+  dispatcher_.post([this]() {


Out of curiosity, why is this posted to the dispatcher instead of running on the current thread? Maybe add a comment?

This is called on from Android API, so not necessarily on the network thread.

Can you clue me in on where it's called unsafely? It looks like it's called from the connection manager's onnetworkmadedefault which is called from InternalEngine::onDefaultNetworkChanged which already switches to the dispatcher context.

Also I'm going to pause here because of quic-specific logic and wait for a discussion of general non-quic plans

Oh, I didn't notice that InternalEngine already post()'ed the callback. E-M only has one dispatcher, so probably we can omit post()'ing here. But theoretically the observer registry is one per worker thread (owned by ThreadLocalClusterManagerImpl), and its onNetworkMadeDefault() is called on the main thread even though InternalEngine already switches to the dispatcher context via post(). On the other hand, E-M only has one network thread and dispatcher. I'm wondering if it's clearer to explicitly do post() here to fit the threading model in Envoy core or skip doing post() given this is E-M code.

I removed the post() here and add assert on isThreadSafe(). instead

RyanTheOptimist · 2024-08-26T15:39:11Z

envoy/upstream/cluster_manager.h

+#else
+// Dumb definitions of QUIC classes if QUICHE is compiled out.
+class EnvoyQuicNetworkObserverRegistryFactory {};
+class EnvoyQuicNetworkObserverRegistry {};


Actually, I think we should handle this conditional definition at the place where these classes are defined. Perhaps instead of conditionally building the .h/.cc files, we could wrap their contents in #ifdef so that they are always defined and we can avoid this conditional logic outside of that class. WDYT?

RyanTheOptimist · 2024-08-26T15:41:18Z

source/common/conn_pool/conn_pool_base.h


  void onConnectionEvent(ActiveClient& client, absl::string_view failure_reason,
-                         Network::ConnectionEvent event);
+                         Network::ConnectionEvent event, bool purge_pending_streams);


nit: please add a comment which explains how purge_pending_streams works (or perhaps why we might set it to true sometimes).

I might be misreading this, but it seems like if purge_pending_streams is true then we end up calling purgePendingStreams() which calls onPoolFailure(). Is the comment backwards, or am I misunderstanding something?

Woops, it is backwards.

RyanTheOptimist · 2024-08-26T16:09:56Z

source/common/http/conn_pool_base.cc

    if (codec_client_->numActiveRequests() == 0) {
+      if (codec_client_->protocol() == Protocol::Http3 && error_code == GoAwayErrorCode::Other) {
+        // This must be network change because QUIC GOAWAY frame doesn't have error code.
+        close_after_network_change_ = true;


Why do we do this only for H3? (Maybe add a comment?)

This is how ActiveClient knows about the upcoming connection close is caused by network change instead of a real GOAWAY frame. And we can only infer it in HTTP/3 because H3 GOAWAY frame should trigger onGoAway() with GoAwayErrorCode::NoError. In this PR QuicNetworkConnectivityObserver::onNetworkChanged() calls this with GoAwayErrorCode::Other.

Hm. I see. And is onNetworkChanged() the only caller which passes in Other? This feels a big like spooky action at a distance. We could potentially add an explicit argument to onGoaway() or add a new enum value, though both are possibly used in lots of places. I don't want to block if we're sure this works, but it feels a bit sketchy.

I was debating between these options. The current way is indeed sketchy, but at least works. I also checked the usage of GoAwayErrorCode and found that a lot of places use if (NoError) {...} else {...} or if (!NoError) {...}, so I hesitated about adding a new enum value.
The remaining alternatives are changing the existing Http::ConnectionCallbacks::onGoAway() interface or adding a new interface to one of the interfaces ConnectionPool::ActiveClient inherits from. These are all used in a lot of places. And onGoAway() is also used in server code where network change doesn't make sense. Which one do you think we should pursue?

Hm... What if we add a new interface for ActiveClient to implement. This new interface could have an onNetworkChanged() method. Then QuicHttpClientConnectionImpl can take an instance of this interface (which would be ActiveClient, of course) where it can call some new setNetworkChangedCallback() method on the EnvoyQuicClientSession. Would that work?

I added a new callback interface for Network::ClientConnection to achieve this.

RyanTheOptimist · 2024-08-26T16:15:28Z

source/common/http/conn_pool_base.h

  HttpConnPoolImplBase& parent() { return *static_cast<HttpConnPoolImplBase*>(&parent_); }

  Http::CodecClientPtr codec_client_;
+  bool close_after_network_change_{false};


nit: comment

RyanTheOptimist · 2024-08-27T19:20:56Z

/wait

Signed-off-by: Dan Zhang <danzh@google.com>

RyanTheOptimist · 2025-09-15T21:00:24Z

CI seems to be failing
/wait

danzh2010 · 2025-09-16T14:07:41Z

/wait

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2025-10-09T18:36:42Z

I split this PR into smaller ones. The current one is simply refining QuicNetworkConnectivityObserver interface. The rest of the plumbing is moved into a follow up PR. PTAL

Signed-off-by: Dan Zhang <danzh@google.com>

abeyad · 2025-10-09T19:22:04Z

source/common/quic/quic_network_connectivity_observer.h

 #include <memory>

-#include "source/common/common/logger.h"
+using NetworkHandle = int64_t;


shouldn't this be moved to inside the Envoy::Quic namespace so it is scoped?

Moved it into Envoy namespace.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2025-10-10T00:59:39Z

Addressed test coverage. PTAL!

abeyad · 2025-10-10T13:29:05Z

This is core code, so please also wait for Ryan's approval

danzh1989 added 7 commits August 7, 2024 21:58

everything build

959cd4c

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'main' into networkchange

4b969c9

Signed-off-by: Dan Zhang <danzh@google.com>

finish plumbing

242dc5e

Signed-off-by: Dan Zhang <danzh@google.com>

add tests

28e805a

Signed-off-by: Dan Zhang <danzh@google.com>

comments

a582d79

Signed-off-by: Dan Zhang <danzh@google.com>

add runtime guard

1e57860

Signed-off-by: Dan Zhang <danzh@google.com>

change log

e3663b2

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 requested review from RyanTheOptimist, abeyad, alyssawilk and fredyw as code owners August 21, 2024 17:01

danzh1989 added 3 commits August 21, 2024 17:51

fix build

ccd0dd2

Signed-off-by: Dan Zhang <danzh@google.com>

fix build again

7e8e3d3

Signed-off-by: Dan Zhang <danzh@google.com>

more fix

84eeb68

Signed-off-by: Dan Zhang <danzh@google.com>

danzh1989 added 3 commits August 22, 2024 15:01

fix compile option build

2337129

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'main' into networkchange

9725172

Signed-off-by: Dan Zhang <danzh@google.com>

test coverage

845c19d

Signed-off-by: Dan Zhang <danzh@google.com>

fix build

462e7bd

Signed-off-by: Dan Zhang <danzh@google.com>

wbpcode assigned alyssawilk Aug 23, 2024

fix compile option build

76e0894

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot assigned RyanTheOptimist Aug 23, 2024

RyanTheOptimist reviewed Aug 26, 2024

View reviewed changes

observer interface change

ee6a0b4

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Sep 12, 2025

more interface changes

bd3fd65

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot added the waiting label Sep 15, 2025

danzh1989 added 4 commits October 3, 2025 15:47

Merge branch 'main' into networkchange

6815335

Signed-off-by: Dan Zhang <danzh@google.com>

revert transient connect failure handling

cdc290d

Signed-off-by: Dan Zhang <danzh@google.com>

remove quic_network_connectivity_observer_impl_lib

f668a04

Signed-off-by: Dan Zhang <danzh@google.com>

build target

73d0b0e

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Oct 3, 2025

danzh1989 added 5 commits October 9, 2025 17:53

revert tests

47d708f

Signed-off-by: Dan Zhang <danzh@google.com>

revert upstream BUILD

8c576fa

Signed-off-by: Dan Zhang <danzh@google.com>

revert mobile changes

6ab8b05

Signed-off-by: Dan Zhang <danzh@google.com>

a disconnect interface

3dba5e6

Signed-off-by: Dan Zhang <danzh@google.com>

revert runtime

809ee7e

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 changed the title ~~quic: propagate default network change event to downstream connections~~ quic: change QuicNetworkConnectivityObserver interface Oct 9, 2025

danzh2010 assigned abeyad and unassigned alyssawilk Oct 9, 2025

fix build

7c31d1e

Signed-off-by: Dan Zhang <danzh@google.com>

abeyad previously approved these changes Oct 9, 2025

View reviewed changes

test coverage

f22a3d9

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed abeyad’s stale review via f22a3d9 October 9, 2025 20:32

abeyad approved these changes Oct 10, 2025

View reviewed changes

RyanTheOptimist approved these changes Oct 10, 2025

View reviewed changes

RyanTheOptimist merged commit a61a0dd into envoyproxy:main Oct 10, 2025
25 checks passed

danzh2010 mentioned this pull request Oct 15, 2025

Notify existing upstream QUIC connections of network changes #36059

Closed

Conversation

danzh2010 commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 21, 2024

Uh oh!

danzh2010 commented Aug 22, 2024

Uh oh!

danzh2010 commented Aug 22, 2024

Uh oh!

danzh2010 commented Aug 22, 2024

Uh oh!

danzh2010 commented Aug 23, 2024

Uh oh!

danzh2010 commented Aug 23, 2024

Uh oh!

danzh2010 commented Aug 23, 2024

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist commented Aug 27, 2024

Uh oh!

RyanTheOptimist commented Sep 15, 2025

danzh2010 commented Aug 21, 2024 •

edited

Loading