Copilot commented on code in PR #12727: URL: https://github.com/apache/trafficserver/pull/12727#discussion_r2587273201
########## tests/gold_tests/pluginTest/tsapi/test_TSHttpTxnServerAddrSet_retry.cc: ########## @@ -0,0 +1,145 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * Test plugin to reproduce issue #12611 + * + * This plugin sets server addresses via TSHttpTxnServerAddrSet() in the OS_DNS hook. + * On first call, it sets a non-routable address that will fail to connect. + * On retry (if OS_DNS is called again), it sets a working address. + * + * BUG: On master, the OS_DNS hook is NOT called again on retry, so the connection + * keeps failing with the bad address. + */ + +#include <ts/ts.h> + +#include <arpa/inet.h> +#include <netinet/in.h> +#include <sys/socket.h> + +#include <atomic> +#include <cstdlib> + +namespace +{ + +DbgCtl dbg_ctl{"test_TSHttpTxnServerAddrSet_retry"}; + +// Track how many times OS_DNS was called (global counter for simplicity) +std::atomic<int> g_os_dns_call_count{0}; + +/** Handler for OS_DNS hook */ +int +handle_os_dns(TSCont /* cont */, TSEvent event, void *edata) +{ + if (event != TS_EVENT_HTTP_OS_DNS) { + TSError("[TSHttpTxnServerAddrSet_retry] Unexpected event: %d", event); + return TS_ERROR; + } + + TSHttpTxn txnp = (TSHttpTxn)edata; + int call_count = ++g_os_dns_call_count; + + Dbg(dbg_ctl, "OS_DNS hook called, count=%d", call_count); + TSError("[TSHttpTxnServerAddrSet_retry] OS_DNS hook called, count=%d", call_count); + + // First call: set a bad address (192.0.2.1 is TEST-NET-1, non-routable) + // Second call: set a good address (localhost) + const char *addr_str; + int port; + + if (call_count == 1) { + addr_str = "192.0.2.1"; // Non-routable - will fail + port = 80; + TSError("[TSHttpTxnServerAddrSet_retry] Attempt 1: Setting BAD address %s:%d (will fail)", addr_str, port); + } else { + addr_str = "127.0.0.1"; // Localhost - should work + port = 8080; + TSError("[TSHttpTxnServerAddrSet_retry] Attempt %d: Setting GOOD address %s:%d (should work)", call_count, addr_str, port); + } + + // Create sockaddr + struct sockaddr_in sa; + sa.sin_family = AF_INET; + sa.sin_port = htons(port); + if (inet_pton(AF_INET, addr_str, &sa.sin_addr) != 1) { + TSError("[TSHttpTxnServerAddrSet_retry] Invalid address %s", addr_str); + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_ERROR); + return TS_ERROR; + } + + // Set the server address + if (TSHttpTxnServerAddrSet(txnp, reinterpret_cast<struct sockaddr const *>(&sa)) != TS_SUCCESS) { + TSError("[TSHttpTxnServerAddrSet_retry] Failed to set server address to %s:%d", addr_str, port); + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_ERROR); + return TS_ERROR; + } + + Dbg(dbg_ctl, "Set server address to %s:%d", addr_str, port); + + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE); + return TS_SUCCESS; +} + +/** Handler for TXN_CLOSE hook - report results */ +int +handle_txn_close(TSCont /* cont */, TSEvent event, void *edata) +{ + if (event != TS_EVENT_HTTP_TXN_CLOSE) { + return TS_ERROR; + } + + TSHttpTxn txnp = (TSHttpTxn)edata; + int call_count = g_os_dns_call_count.load(); Review Comment: The atomic counter is loaded in the TXN_CLOSE handler, which could read a value that includes increments from other concurrent transactions. This will give incorrect results if multiple requests are processed at the same time. With transaction-specific storage, this would read the count specific to this transaction only. ########## tests/gold_tests/pluginTest/tsapi/test_TSHttpTxnServerAddrSet_retry.cc: ########## @@ -0,0 +1,145 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * Test plugin to reproduce issue #12611 + * + * This plugin sets server addresses via TSHttpTxnServerAddrSet() in the OS_DNS hook. + * On first call, it sets a non-routable address that will fail to connect. + * On retry (if OS_DNS is called again), it sets a working address. + * + * BUG: On master, the OS_DNS hook is NOT called again on retry, so the connection + * keeps failing with the bad address. + */ + +#include <ts/ts.h> + +#include <arpa/inet.h> +#include <netinet/in.h> +#include <sys/socket.h> + +#include <atomic> +#include <cstdlib> + +namespace +{ + +DbgCtl dbg_ctl{"test_TSHttpTxnServerAddrSet_retry"}; + +// Track how many times OS_DNS was called (global counter for simplicity) +std::atomic<int> g_os_dns_call_count{0}; + +/** Handler for OS_DNS hook */ +int +handle_os_dns(TSCont /* cont */, TSEvent event, void *edata) +{ + if (event != TS_EVENT_HTTP_OS_DNS) { + TSError("[TSHttpTxnServerAddrSet_retry] Unexpected event: %d", event); + return TS_ERROR; + } + + TSHttpTxn txnp = (TSHttpTxn)edata; + int call_count = ++g_os_dns_call_count; + + Dbg(dbg_ctl, "OS_DNS hook called, count=%d", call_count); + TSError("[TSHttpTxnServerAddrSet_retry] OS_DNS hook called, count=%d", call_count); + + // First call: set a bad address (192.0.2.1 is TEST-NET-1, non-routable) + // Second call: set a good address (localhost) + const char *addr_str; + int port; + + if (call_count == 1) { + addr_str = "192.0.2.1"; // Non-routable - will fail + port = 80; + TSError("[TSHttpTxnServerAddrSet_retry] Attempt 1: Setting BAD address %s:%d (will fail)", addr_str, port); + } else { + addr_str = "127.0.0.1"; // Localhost - should work + port = 8080; + TSError("[TSHttpTxnServerAddrSet_retry] Attempt %d: Setting GOOD address %s:%d (should work)", call_count, addr_str, port); + } + + // Create sockaddr + struct sockaddr_in sa; + sa.sin_family = AF_INET; + sa.sin_port = htons(port); + if (inet_pton(AF_INET, addr_str, &sa.sin_addr) != 1) { + TSError("[TSHttpTxnServerAddrSet_retry] Invalid address %s", addr_str); + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_ERROR); + return TS_ERROR; + } + + // Set the server address + if (TSHttpTxnServerAddrSet(txnp, reinterpret_cast<struct sockaddr const *>(&sa)) != TS_SUCCESS) { + TSError("[TSHttpTxnServerAddrSet_retry] Failed to set server address to %s:%d", addr_str, port); + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_ERROR); + return TS_ERROR; + } + + Dbg(dbg_ctl, "Set server address to %s:%d", addr_str, port); + + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE); + return TS_SUCCESS; +} + +/** Handler for TXN_CLOSE hook - report results */ +int +handle_txn_close(TSCont /* cont */, TSEvent event, void *edata) +{ + if (event != TS_EVENT_HTTP_TXN_CLOSE) { Review Comment: [nitpick] The TXN_CLOSE handler doesn't validate the event type before processing, unlike the OS_DNS handler. For consistency and defensive programming, consider adding a check like: ```cpp if (event != TS_EVENT_HTTP_TXN_CLOSE) { TSError("[TSHttpTxnServerAddrSet_retry] Unexpected event in TXN_CLOSE handler: %d", event); return TS_ERROR; } ``` ```suggestion if (event != TS_EVENT_HTTP_TXN_CLOSE) { TSError("[TSHttpTxnServerAddrSet_retry] Unexpected event in TXN_CLOSE handler: %d", event); ``` ########## tests/gold_tests/pluginTest/tsapi/test_TSHttpTxnServerAddrSet_retry.cc: ########## @@ -0,0 +1,145 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * Test plugin to reproduce issue #12611 + * + * This plugin sets server addresses via TSHttpTxnServerAddrSet() in the OS_DNS hook. + * On first call, it sets a non-routable address that will fail to connect. + * On retry (if OS_DNS is called again), it sets a working address. + * + * BUG: On master, the OS_DNS hook is NOT called again on retry, so the connection + * keeps failing with the bad address. + */ + +#include <ts/ts.h> + +#include <arpa/inet.h> +#include <netinet/in.h> +#include <sys/socket.h> + +#include <atomic> +#include <cstdlib> + +namespace +{ + +DbgCtl dbg_ctl{"test_TSHttpTxnServerAddrSet_retry"}; + +// Track how many times OS_DNS was called (global counter for simplicity) +std::atomic<int> g_os_dns_call_count{0}; + +/** Handler for OS_DNS hook */ +int +handle_os_dns(TSCont /* cont */, TSEvent event, void *edata) +{ + if (event != TS_EVENT_HTTP_OS_DNS) { + TSError("[TSHttpTxnServerAddrSet_retry] Unexpected event: %d", event); + return TS_ERROR; + } + + TSHttpTxn txnp = (TSHttpTxn)edata; + int call_count = ++g_os_dns_call_count; + + Dbg(dbg_ctl, "OS_DNS hook called, count=%d", call_count); + TSError("[TSHttpTxnServerAddrSet_retry] OS_DNS hook called, count=%d", call_count); + + // First call: set a bad address (192.0.2.1 is TEST-NET-1, non-routable) + // Second call: set a good address (localhost) + const char *addr_str; + int port; + + if (call_count == 1) { + addr_str = "192.0.2.1"; // Non-routable - will fail + port = 80; + TSError("[TSHttpTxnServerAddrSet_retry] Attempt 1: Setting BAD address %s:%d (will fail)", addr_str, port); + } else { + addr_str = "127.0.0.1"; // Localhost - should work + port = 8080; + TSError("[TSHttpTxnServerAddrSet_retry] Attempt %d: Setting GOOD address %s:%d (should work)", call_count, addr_str, port); + } + + // Create sockaddr + struct sockaddr_in sa; + sa.sin_family = AF_INET; + sa.sin_port = htons(port); + if (inet_pton(AF_INET, addr_str, &sa.sin_addr) != 1) { + TSError("[TSHttpTxnServerAddrSet_retry] Invalid address %s", addr_str); + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_ERROR); + return TS_ERROR; + } + + // Set the server address + if (TSHttpTxnServerAddrSet(txnp, reinterpret_cast<struct sockaddr const *>(&sa)) != TS_SUCCESS) { + TSError("[TSHttpTxnServerAddrSet_retry] Failed to set server address to %s:%d", addr_str, port); + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_ERROR); + return TS_ERROR; + } + + Dbg(dbg_ctl, "Set server address to %s:%d", addr_str, port); + + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE); + return TS_SUCCESS; +} + +/** Handler for TXN_CLOSE hook - report results */ +int +handle_txn_close(TSCont /* cont */, TSEvent event, void *edata) +{ + if (event != TS_EVENT_HTTP_TXN_CLOSE) { + return TS_ERROR; + } + + TSHttpTxn txnp = (TSHttpTxn)edata; + int call_count = g_os_dns_call_count.load(); Review Comment: The atomic counter loaded here could include increments from other concurrent transactions if they exist. This will give incorrect results for the specific transaction being closed. With transaction-specific storage, this would read the count specific to this transaction only. ########## tests/gold_tests/pluginTest/tsapi/test_TSHttpTxnServerAddrSet_retry.cc: ########## @@ -0,0 +1,145 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * Test plugin to reproduce issue #12611 + * + * This plugin sets server addresses via TSHttpTxnServerAddrSet() in the OS_DNS hook. + * On first call, it sets a non-routable address that will fail to connect. + * On retry (if OS_DNS is called again), it sets a working address. + * + * BUG: On master, the OS_DNS hook is NOT called again on retry, so the connection + * keeps failing with the bad address. + */ + +#include <ts/ts.h> + +#include <arpa/inet.h> +#include <netinet/in.h> +#include <sys/socket.h> + +#include <atomic> +#include <cstdlib> + +namespace +{ + +DbgCtl dbg_ctl{"test_TSHttpTxnServerAddrSet_retry"}; + +// Track how many times OS_DNS was called (global counter for simplicity) +std::atomic<int> g_os_dns_call_count{0}; Review Comment: The global atomic counter `g_os_dns_call_count` is shared across all transactions. While this test may only make a single request, the plugin code should handle concurrent transactions correctly. If multiple requests are processed concurrently (e.g., if the test is extended or if there are background requests), the counter will be incremented by all of them, making the results unreliable. Consider using transaction-specific storage (e.g., via `TSHttpTxnArgSet`/`TSHttpTxnArgGet`) to track the call count per transaction, or use a mutex-protected map keyed by transaction pointer. ########## tests/gold_tests/pluginTest/tsapi/test_TSHttpTxnServerAddrSet_retry.cc: ########## @@ -0,0 +1,145 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * Test plugin to reproduce issue #12611 + * + * This plugin sets server addresses via TSHttpTxnServerAddrSet() in the OS_DNS hook. + * On first call, it sets a non-routable address that will fail to connect. + * On retry (if OS_DNS is called again), it sets a working address. + * + * BUG: On master, the OS_DNS hook is NOT called again on retry, so the connection + * keeps failing with the bad address. + */ + +#include <ts/ts.h> + +#include <arpa/inet.h> +#include <netinet/in.h> +#include <sys/socket.h> + +#include <atomic> +#include <cstdlib> + +namespace +{ + +DbgCtl dbg_ctl{"test_TSHttpTxnServerAddrSet_retry"}; + +// Track how many times OS_DNS was called (global counter for simplicity) +std::atomic<int> g_os_dns_call_count{0}; + +/** Handler for OS_DNS hook */ +int +handle_os_dns(TSCont /* cont */, TSEvent event, void *edata) +{ + if (event != TS_EVENT_HTTP_OS_DNS) { + TSError("[TSHttpTxnServerAddrSet_retry] Unexpected event: %d", event); + return TS_ERROR; + } + + TSHttpTxn txnp = (TSHttpTxn)edata; + int call_count = ++g_os_dns_call_count; + + Dbg(dbg_ctl, "OS_DNS hook called, count=%d", call_count); + TSError("[TSHttpTxnServerAddrSet_retry] OS_DNS hook called, count=%d", call_count); + + // First call: set a bad address (192.0.2.1 is TEST-NET-1, non-routable) + // Second call: set a good address (localhost) + const char *addr_str; + int port; + + if (call_count == 1) { + addr_str = "192.0.2.1"; // Non-routable - will fail + port = 80; + TSError("[TSHttpTxnServerAddrSet_retry] Attempt 1: Setting BAD address %s:%d (will fail)", addr_str, port); + } else { + addr_str = "127.0.0.1"; // Localhost - should work + port = 8080; + TSError("[TSHttpTxnServerAddrSet_retry] Attempt %d: Setting GOOD address %s:%d (should work)", call_count, addr_str, port); + } Review Comment: The logic `if (call_count == 1)` only works correctly if there's a single transaction. With multiple concurrent or sequential transactions, the second transaction's first OS_DNS call would see `call_count > 1` and incorrectly set the "good" address instead of the "bad" address. For proper per-transaction behavior, use transaction-specific storage to track each transaction's call count independently. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
