From 364ee8512946b7a9b094750f89d215f46e5f5172 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 15 Sep 2022 17:38:49 +0000 Subject: [PATCH 1/5] Disable MDNS lookup on first message deliver failure --- src/messaging/BUILD.gn | 7 +++++++ src/messaging/ReliableMessageMgr.cpp | 2 ++ src/messaging/ReliableMessageProtocolConfig.h | 16 ++++++++++++++++ 3 files changed, 25 insertions(+) diff --git a/src/messaging/BUILD.gn b/src/messaging/BUILD.gn index f01b749d3f6d17..1615475353918a 100644 --- a/src/messaging/BUILD.gn +++ b/src/messaging/BUILD.gn @@ -17,6 +17,8 @@ import("//build_overrides/chip.gni") declare_args() { # Allows to change time between retries during the case session chip_case_retry_delta = "" + + chip_config_mrp_on_first_msg_fail_do_mdsn_lookup = "" } defines = [] @@ -26,6 +28,11 @@ if (chip_case_retry_delta != "") { [ "CHIP_CONFIG_MRP_LOCAL_ACTIVE_RETRY_INTERVAL=${chip_case_retry_delta}" ] } +if (chip_config_mrp_on_first_msg_fail_do_mdsn_lookup != "") { + defines += + [ "CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP=${chip_config_mrp_on_first_msg_fail_do_mdsn_lookup}" ] +} + source_set("messaging_mrp_config") { sources = [ "ReliableMessageProtocolConfig.h" ] diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index e745e54d46665e..9859565b4249f8 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -316,6 +316,7 @@ CHIP_ERROR ReliableMessageMgr::SendFromRetransTable(RetransTableEntry * entry) if (err == CHIP_NO_ERROR) { +#if CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP const ExchangeManager * exchangeMgr = entry->ec->GetExchangeMgr(); // TODO: investigate why in ReliableMessageMgr::CheckResendApplicationMessageWithPeerExchange unit test released exchange // context with mExchangeMgr==nullptr is used. @@ -329,6 +330,7 @@ CHIP_ERROR ReliableMessageMgr::SendFromRetransTable(RetransTableEntry * entry) mSessionUpdateDelegate->UpdatePeerAddress(entry->ec->GetSessionHandle()->GetPeer()); } } +#endif // CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP } else { diff --git a/src/messaging/ReliableMessageProtocolConfig.h b/src/messaging/ReliableMessageProtocolConfig.h index f05f4b05d9cc97..c92705e0fa183d 100644 --- a/src/messaging/ReliableMessageProtocolConfig.h +++ b/src/messaging/ReliableMessageProtocolConfig.h @@ -46,6 +46,22 @@ namespace chip { #define CHIP_CONFIG_MRP_LOCAL_ACTIVE_RETRY_INTERVAL (300_ms32) #endif // CHIP_CONFIG_MRP_LOCAL_ACTIVE_RETRY_INTERVAL +/** + * @def CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP + * + * @brief + * Should an address lookup of the peer happen on every first message that fails + * to send on the link. + * + * The default value selected to not perform lookup was for the following reasons: + * 1. The likelihood of the IP address changing right away is extremely slim. + * 2. On bad links, there resolutions occur extremely frequently, making the link + * worse. + */ +#ifndef CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP +#define CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP 0 +#endif // CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP + /** * @def CHIP_CONFIG_MRP_LOCAL_IDLE_RETRY_INTERVAL * From 963f4ba498ce5f3765bff68390a094cc54d7b245 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 15 Sep 2022 17:46:26 +0000 Subject: [PATCH 2/5] Restyle --- src/messaging/BUILD.gn | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/messaging/BUILD.gn b/src/messaging/BUILD.gn index 1615475353918a..a2863dbc12c2a4 100644 --- a/src/messaging/BUILD.gn +++ b/src/messaging/BUILD.gn @@ -29,8 +29,7 @@ if (chip_case_retry_delta != "") { } if (chip_config_mrp_on_first_msg_fail_do_mdsn_lookup != "") { - defines += - [ "CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP=${chip_config_mrp_on_first_msg_fail_do_mdsn_lookup}" ] + defines += [ "CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP=${chip_config_mrp_on_first_msg_fail_do_mdsn_lookup}" ] } source_set("messaging_mrp_config") { From 914e3f781ded18505c5be6fe12dc6bcf646c93ea Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 15 Sep 2022 17:48:58 +0000 Subject: [PATCH 3/5] Reorder config entry --- src/messaging/ReliableMessageProtocolConfig.h | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/messaging/ReliableMessageProtocolConfig.h b/src/messaging/ReliableMessageProtocolConfig.h index c92705e0fa183d..aff9ab0ddb565e 100644 --- a/src/messaging/ReliableMessageProtocolConfig.h +++ b/src/messaging/ReliableMessageProtocolConfig.h @@ -46,22 +46,6 @@ namespace chip { #define CHIP_CONFIG_MRP_LOCAL_ACTIVE_RETRY_INTERVAL (300_ms32) #endif // CHIP_CONFIG_MRP_LOCAL_ACTIVE_RETRY_INTERVAL -/** - * @def CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP - * - * @brief - * Should an address lookup of the peer happen on every first message that fails - * to send on the link. - * - * The default value selected to not perform lookup was for the following reasons: - * 1. The likelihood of the IP address changing right away is extremely slim. - * 2. On bad links, there resolutions occur extremely frequently, making the link - * worse. - */ -#ifndef CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP -#define CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP 0 -#endif // CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP - /** * @def CHIP_CONFIG_MRP_LOCAL_IDLE_RETRY_INTERVAL * @@ -87,6 +71,22 @@ namespace chip { #define CHIP_CONFIG_RMP_DEFAULT_ACK_TIMEOUT (200_ms32) #endif // CHIP_CONFIG_RMP_DEFAULT_ACK_TIMEOUT +/** + * @def CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP + * + * @brief + * Should an address lookup of the peer happen on every first message that fails + * to send on the link. + * + * The default value selected to not perform lookup was for the following reasons: + * 1. The likelihood of the IP address changing right away is extremely slim. + * 2. On bad links, there resolutions occur extremely frequently, making the link + * worse. + */ +#ifndef CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP +#define CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP 0 +#endif // CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP + /** * @def CHIP_CONFIG_RMP_RETRANS_TABLE_SIZE * From d9204f3dd580364234d1d9afd5cd7a6dfd8d86c9 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 15 Sep 2022 18:05:24 +0000 Subject: [PATCH 4/5] Address PR comments and clean up config comment --- src/messaging/BUILD.gn | 6 +++--- src/messaging/ReliableMessageMgr.cpp | 4 ++-- src/messaging/ReliableMessageProtocolConfig.h | 20 +++++++++++-------- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/messaging/BUILD.gn b/src/messaging/BUILD.gn index a2863dbc12c2a4..bf363c7239db71 100644 --- a/src/messaging/BUILD.gn +++ b/src/messaging/BUILD.gn @@ -18,7 +18,7 @@ declare_args() { # Allows to change time between retries during the case session chip_case_retry_delta = "" - chip_config_mrp_on_first_msg_fail_do_mdsn_lookup = "" + chip_config_resolve_peer_on_first_transmit_failure = "" } defines = [] @@ -28,8 +28,8 @@ if (chip_case_retry_delta != "") { [ "CHIP_CONFIG_MRP_LOCAL_ACTIVE_RETRY_INTERVAL=${chip_case_retry_delta}" ] } -if (chip_config_mrp_on_first_msg_fail_do_mdsn_lookup != "") { - defines += [ "CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP=${chip_config_mrp_on_first_msg_fail_do_mdsn_lookup}" ] +if (chip_config_resolve_peer_on_first_transmit_failure != "") { + defines += [ "CHIP_CONFIG_RESOLVE_PEER_ON_FIRST_TRANSMIT_FAILURE=${chip_config_resolve_peer_on_first_transmit_failure}" ] } source_set("messaging_mrp_config") { diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 9859565b4249f8..f1b297b7b8d799 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -316,7 +316,7 @@ CHIP_ERROR ReliableMessageMgr::SendFromRetransTable(RetransTableEntry * entry) if (err == CHIP_NO_ERROR) { -#if CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP +#if CHIP_CONFIG_RESOLVE_PEER_ON_FIRST_TRANSMIT_FAILURE const ExchangeManager * exchangeMgr = entry->ec->GetExchangeMgr(); // TODO: investigate why in ReliableMessageMgr::CheckResendApplicationMessageWithPeerExchange unit test released exchange // context with mExchangeMgr==nullptr is used. @@ -330,7 +330,7 @@ CHIP_ERROR ReliableMessageMgr::SendFromRetransTable(RetransTableEntry * entry) mSessionUpdateDelegate->UpdatePeerAddress(entry->ec->GetSessionHandle()->GetPeer()); } } -#endif // CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP +#endif // CHIP_CONFIG_RESOLVE_PEER_ON_FIRST_TRANSMIT_FAILURE } else { diff --git a/src/messaging/ReliableMessageProtocolConfig.h b/src/messaging/ReliableMessageProtocolConfig.h index aff9ab0ddb565e..705cf618cb37b3 100644 --- a/src/messaging/ReliableMessageProtocolConfig.h +++ b/src/messaging/ReliableMessageProtocolConfig.h @@ -72,20 +72,24 @@ namespace chip { #endif // CHIP_CONFIG_RMP_DEFAULT_ACK_TIMEOUT /** - * @def CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP + * @def CHIP_CONFIG_RESOLVE_PEER_ON_FIRST_TRANSMIT_FAILURE * * @brief * Should an address lookup of the peer happen on every first message that fails * to send on the link. * - * The default value selected to not perform lookup was for the following reasons: - * 1. The likelihood of the IP address changing right away is extremely slim. - * 2. On bad links, there resolutions occur extremely frequently, making the link - * worse. + * The default value to not perform lookup was selected because most implementations + * of address lookup are not cache the and a request is sent on the link. Failing + * to deliver the first message is far more likely to happen due to lossy link + * than an actual address change where the peer did not reset. In the lossy link + * situation the frequent lookup would make the link even worse. Additionally, + * every message that arrives from a peer updates the address. If the peer has fallen + * off the link due to any other reason, a re-resolve may not achieve an address that + * is reachable, even if a resolve response occurs. */ -#ifndef CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP -#define CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP 0 -#endif // CHIP_CONFIG_MRP_ON_FIRST_MSG_FAIL_DO_MDNS_LOOKUP +#ifndef CHIP_CONFIG_RESOLVE_PEER_ON_FIRST_TRANSMIT_FAILURE +#define CHIP_CONFIG_RESOLVE_PEER_ON_FIRST_TRANSMIT_FAILURE 0 +#endif // CHIP_CONFIG_RESOLVE_PEER_ON_FIRST_TRANSMIT_FAILURE /** * @def CHIP_CONFIG_RMP_RETRANS_TABLE_SIZE From f1faeab386a0c61650b36513960a2d0e2b392002 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 15 Sep 2022 18:17:29 +0000 Subject: [PATCH 5/5] Address PR comments --- src/messaging/ReliableMessageProtocolConfig.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/messaging/ReliableMessageProtocolConfig.h b/src/messaging/ReliableMessageProtocolConfig.h index 705cf618cb37b3..2e253c10bdc3ac 100644 --- a/src/messaging/ReliableMessageProtocolConfig.h +++ b/src/messaging/ReliableMessageProtocolConfig.h @@ -82,10 +82,10 @@ namespace chip { * of address lookup are not cache the and a request is sent on the link. Failing * to deliver the first message is far more likely to happen due to lossy link * than an actual address change where the peer did not reset. In the lossy link - * situation the frequent lookup would make the link even worse. Additionally, - * every message that arrives from a peer updates the address. If the peer has fallen - * off the link due to any other reason, a re-resolve may not achieve an address that - * is reachable, even if a resolve response occurs. + * situation, doing further DNS resolutions on a degraded link can exacerbate that + * problem greatly. Additionally, every message that arrives from a peer updates the + * address. If the peer has fallen off the link due to any other reason, a re-resolve + * may not achieve an address that is reachable, even if a resolve response occurs. */ #ifndef CHIP_CONFIG_RESOLVE_PEER_ON_FIRST_TRANSMIT_FAILURE #define CHIP_CONFIG_RESOLVE_PEER_ON_FIRST_TRANSMIT_FAILURE 0