quic: change QuicNetworkConnectivityObserver interface #35775
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,20 @@ constexpr unsigned int InitialFaultThreshold = 1; | |
| // L7 bytes) before switching socket mode. | ||
| constexpr unsigned int MaxFaultThreshold = 3; | ||
|
|
||
| ConnectivityManagerImpl::ConnectivityManagerImpl(Upstream::ClusterManager& cluster_manager, | ||
| DnsCacheManagerSharedPtr dns_cache_manager) | ||
| : cluster_manager_(cluster_manager), dns_cache_manager_(dns_cache_manager), | ||
| quic_upstream_connection_handle_network_change_(Runtime::runtimeFeatureEnabled( | ||
| "envoy.reloadable_features.quic_upstream_connection_handle_network_change")) { | ||
| #ifdef ENVOY_ENABLE_QUIC | ||
| quic_observer_registry_factory_ = | ||
| std::make_unique<Quic::EnvoyMobileQuicNetworkObserverRegistryFactory>(); | ||
| if (quic_upstream_connection_handle_network_change_) { | ||
| cluster_manager_.createNetworkObserverRegistries(*quic_observer_registry_factory_); | ||
| } | ||
| #endif | ||
| } | ||
|
|
||
| ConnectivityManagerImpl::NetworkState ConnectivityManagerImpl::network_state_{ | ||
| 1, NetworkType::Generic, MaxFaultThreshold, SocketMode::DefaultPreferredNetworkMode, | ||
| Thread::MutexBasicLockable{}}; | ||
|
|
@@ -102,6 +116,18 @@ envoy_netconf_t ConnectivityManagerImpl::setPreferredNetwork(NetworkType network | |
| return network_state_.configuration_key_; | ||
| } | ||
|
|
||
| envoy_netconf_t ConnectivityManagerImpl::onNetworkMadeDefault(NetworkType network) { | ||
| ENVOY_LOG_MISC(trace, "Default network changed to {}", static_cast<int>(network)); | ||
| envoy_netconf_t configuration_key = setPreferredNetwork(network); | ||
| #ifdef ENVOY_ENABLE_QUIC | ||
| for (std::reference_wrapper<Quic::EnvoyMobileQuicNetworkObserverRegistry> registry : | ||
| quic_observer_registry_factory_->getCreatedObserverRegistries()) { | ||
| registry.get().onNetworkMadeDefault(); | ||
| } | ||
| #endif | ||
| return configuration_key; | ||
| } | ||
|
|
||
| void ConnectivityManagerImpl::setProxySettings(ProxySettingsConstSharedPtr new_proxy_settings) { | ||
| if (proxy_settings_ == nullptr && new_proxy_settings != nullptr) { | ||
| ENVOY_LOG_EVENT(info, "netconf_proxy_change", "{}", new_proxy_settings->asString()); | ||
|
|
@@ -305,16 +331,17 @@ Socket::OptionsSharedPtr ConnectivityManagerImpl::getUpstreamSocketOptions(Netwo | |
| network != NetworkType::Generic) { | ||
| return getAlternateInterfaceSocketOptions(network); | ||
| } | ||
| ASSERT(static_cast<int>(network) >= 0 && static_cast<int>(network) < 3); | ||
| auto options = std::make_shared<Socket::Options>(); | ||
| if (!quic_upstream_connection_handle_network_change_) { | ||
|
Contributor
There was a problem hiding this comment. Why are we not doing this when the runtime guard is true?
Contributor
Author
There was a problem hiding this comment. This is the old way of not reusing the existing connections for new request. It actually changes how a request is hashed to a connection pool. Since we are going away connections, we don't need to rely on this. And in the new world, we should still be able to reuse the old connection pools, just not the existing connection in it.
Contributor
There was a problem hiding this comment. Ah, I see. Good point. Nice!
Contributor
There was a problem hiding this comment. so I agree with this as a long term plan but without marking H2 and H1 connections as draining won't this possibly be a regression for non-H3 traffic?
Contributor
Author
There was a problem hiding this comment. That's a good point! I think this feature needs to be enabled when both H2 and H3 support network change. Adding network connectivity observers to H2 is another big change. I would prefer not to do it in this PR. The runtime guard is default false, do you think we can land the H3 support first and then change H2 and flip the runtime guard? I didn't see how H1 is handled in Chromium. Is network change an issue in H1 at all?
Contributor
There was a problem hiding this comment. For H1 in Chrome, I think the connections are simply abruptly terminated when the IP address changes:
Contributor
Author
There was a problem hiding this comment. Filed #36059 for tracking and explanation. |
||
| // Envoy uses the hash signature of overridden socket options to choose a connection pool. | ||
| // Setting a dummy socket option is a hack that allows us to select a different | ||
| // connection pool without materially changing the socket configuration. | ||
| int ttl_value = DEFAULT_IP_TTL + static_cast<int>(network); | ||
| options->push_back(std::make_shared<AddrFamilyAwareSocketOptionImpl>( | ||
| envoy::config::core::v3::SocketOption::STATE_PREBIND, ENVOY_SOCKET_IP_TTL, | ||
| ENVOY_SOCKET_IPV6_UNICAST_HOPS, ttl_value)); | ||
| } | ||
| return options; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| #include "library/common/network/envoy_mobile_quic_network_observer_registry_factory.h" | ||
|
|
||
| #include "source/common/runtime/runtime_features.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Quic { | ||
|
|
||
| void EnvoyMobileQuicNetworkObserverRegistry::onNetworkMadeDefault() { | ||
| ENVOY_LOG_MISC(trace, "Default network changed."); | ||
| ASSERT(Runtime::runtimeFeatureEnabled( | ||
| "envoy.reloadable_features.quic_upstream_connection_handle_network_change")); | ||
| dispatcher_.post([this]() { | ||
|
Contributor
There was a problem hiding this comment. Out of curiosity, why is this posted to the dispatcher instead of running on the current thread? Maybe add a comment?
Contributor
Author
There was a problem hiding this comment. This is called on from Android API, so not necessarily on the network thread.
Contributor
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. Also I'm going to pause here because of quic-specific logic and wait for a discussion of general non-quic plans
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
Author
There was a problem hiding this comment. I removed the post() here and add assert on |
||
| // Retain the existing observers in a list and iterate on the list. | ||
|
Contributor
There was a problem hiding this comment. Why do we need to do this? Does the list get potentially mutated?
Contributor
Author
There was a problem hiding this comment. yes, the conn pool may create new connections as a back fill if the connections on this list gets closed. |
||
| std::list<QuicNetworkConnectivityObserver*> existing_observers; | ||
|
Contributor
There was a problem hiding this comment. nit: a vector with |
||
| for (QuicNetworkConnectivityObserver* observer : registeredQuicObservers()) { | ||
| existing_observers.push_back(observer); | ||
| } | ||
| for (auto* observer : existing_observers) { | ||
|
Contributor
There was a problem hiding this comment. nit: explicit type here (I think |
||
| observer->onNetworkChanged(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| EnvoyQuicNetworkObserverRegistryPtr | ||
| EnvoyMobileQuicNetworkObserverRegistryFactory::createQuicNetworkObserverRegistry( | ||
| Event::Dispatcher& dispatcher) { | ||
| auto result = std::make_unique<EnvoyMobileQuicNetworkObserverRegistry>(dispatcher); | ||
| thread_local_observer_registries_.emplace_back(*result); | ||
| return result; | ||
| } | ||
|
|
||
| } // namespace Quic | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| #pragma once | ||
|
|
||
| #include "source/common/quic/envoy_quic_network_observer_registry_factory.h" | ||
| #include "source/common/quic/quic_network_connectivity_observer.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Quic { | ||
|
|
||
| // A mobile implementation that also handles network change events. | ||
| class EnvoyMobileQuicNetworkObserverRegistry : public EnvoyQuicNetworkObserverRegistry { | ||
| public: | ||
| explicit EnvoyMobileQuicNetworkObserverRegistry(Event::Dispatcher& dispatcher) | ||
| : dispatcher_(dispatcher) {} | ||
|
|
||
| void onNetworkMadeDefault(); | ||
|
danzh2010 marked this conversation as resolved.
Outdated
|
||
|
|
||
| private: | ||
| Event::Dispatcher& dispatcher_; | ||
| }; | ||
|
|
||
| class EnvoyMobileQuicNetworkObserverRegistryFactory | ||
| : public EnvoyQuicNetworkObserverRegistryFactory { | ||
| public: | ||
| EnvoyQuicNetworkObserverRegistryPtr | ||
| createQuicNetworkObserverRegistry(Event::Dispatcher& dispatcher) override; | ||
|
|
||
| std::list<std::reference_wrapper<EnvoyMobileQuicNetworkObserverRegistry>>& | ||
| getCreatedObserverRegistries() { | ||
| return thread_local_observer_registries_; | ||
| } | ||
|
|
||
| private: | ||
| std::list<std::reference_wrapper<EnvoyMobileQuicNetworkObserverRegistry>> | ||
|
Contributor
There was a problem hiding this comment. ditto about using a vector instead of a list. |
||
| thread_local_observer_registries_; | ||
| }; | ||
|
|
||
| using EnvoyMobileQuicNetworkObserverRegistryPtr = | ||
| std::unique_ptr<EnvoyMobileQuicNetworkObserverRegistry>; | ||
|
|
||
| } // namespace Quic | ||
| } // namespace Envoy | ||
Uh oh!
There was an error while loading. Please reload this page.