Conversation
📝 WalkthroughWalkthroughThis pull request encompasses updates across several files in the FairMQ project, primarily focusing on copyright year updates, dependency version changes, and modifications to network-related functionality. The changes include updating the copyright years to 2025, modifying the GTest package version, refactoring the Changes
Sequence DiagramsequenceDiagram
participant Client
participant Network
participant Resolver
Client->>Network: getIpFromHostname(hostname)
Network->>Resolver: Async resolve hostname
Resolver-->>Network: Return IP address
alt IP address found
Network-->>Client: Return IP address
else No IP address
Network-->>Client: Return empty string
Network->>Network: Log warning
end
The sequence diagram illustrates the new asynchronous hostname resolution process implemented in the Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fairmq/tools/Network.cxx (1)
165-173: LGTM: Improved IP resolution implementationThe changes enhance the IP resolution implementation by:
- Using explicit IPv4 resolution with
tcp::v4()- Adding proper empty result handling
- Using a more concise way to extract the address
Consider adding a unit test to verify the IPv4-only behavior:
Would you like me to help create a unit test that verifies:
- IPv4 addresses are correctly resolved
- IPv6 addresses are properly excluded
- Empty results are handled gracefully
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmake/FairMQDependencies.cmake(2 hunks)fairmq/tools/Network.cxx(3 hunks)fairmq/tools/Process.cxx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- cmake/FairMQDependencies.cmake
🔇 Additional comments (4)
fairmq/tools/Process.cxx (2)
2-2: LGTM: Copyright year updatedThe copyright year has been updated to include 2025.
Line range hint
67-157: LGTM: Proper migration from io_service to io_contextThe changes correctly update the Boost.Asio I/O service type from the deprecated
io_serviceto the newerio_context. This is a necessary change for Boost 1.87 compatibility.Let's verify there are no remaining uses of
io_service:✅ Verification successful
Migration to io_context is complete and consistent
The codebase has been fully migrated to use
io_context, with no remaining instances of the deprecatedio_service. The changes are consistent across all files using Boost.Asio.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of io_service rg "io_service" --type cppLength of output: 26
Script:
#!/bin/bash # First verify we have C++ files echo "=== C++ files in repository ===" fd -e cpp -e cxx -e h -e hpp echo -e "\n=== Case-insensitive search for io_service ===" rg -i "io_service" echo -e "\n=== Verify we can find other Boost.Asio types ===" rg "boost::asio|asio::"Length of output: 7059
fairmq/tools/Network.cxx (2)
2-2: LGTM: Copyright year updatedThe copyright year has been updated to include 2025.
11-11: LGTM: Added Strings.h includeThe addition of
fairmq/tools/Strings.his necessary for theToStringfunction used in the updated implementation.
Yes |
|
Tip For best results, initiate chat on the files or code changes. @dennisklein Here's a unit test file for the Network functionality. Create this as /********************************************************************************
* Copyright (C) 2017-2025 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH *
* *
* This software is distributed under the terms of the *
* GNU Lesser General Public Licence (LGPL) version 3, *
* copied verbatim in the file "LICENSE" *
********************************************************************************/
#include <fairmq/tools/Network.h>
#include <boost/test/unit_test.hpp>
using namespace std;
using namespace fair::mq::tools;
namespace {
BOOST_AUTO_TEST_SUITE(Network_test)
BOOST_AUTO_TEST_CASE(ResolveIPv4Address)
{
// This is a special test domain that always resolves to 127.0.0.1
string result = getIpFromHostname("localhost");
BOOST_TEST(!result.empty());
BOOST_TEST(result == "127.0.0.1");
}
BOOST_AUTO_TEST_CASE(HandleEmptyResult)
{
// Non-existent domain should return empty string
string result = getIpFromHostname("non.existent.domain.invalid");
BOOST_TEST(result.empty());
}
BOOST_AUTO_TEST_CASE(IPv6AddressExclusion)
{
// ipv6.google.com is guaranteed to have only IPv6 addresses
string result = getIpFromHostname("ipv6.google.com");
BOOST_TEST(result.empty());
}
BOOST_AUTO_TEST_SUITE_END()
}This test suite includes three test cases:
You'll need to add this test to your CMake configuration. Let me know if you need help with that or if you'd like to modify these test cases. 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 5722 Script: Length of output: 68 |
|
@coderabbitai Add the proposed tests to the file |
|
Tip For best results, initiate chat on the files or code changes. @dennisklein Here are the additional test cases for the Network functionality, added to TEST(Tools, NetworkIPv4Resolution)
{
string result = fair::mq::tools::getIpFromHostname("localhost");
EXPECT_FALSE(result.empty());
EXPECT_EQ(result, "127.0.0.1");
// Test a well-known public DNS that should resolve to an IPv4
result = fair::mq::tools::getIpFromHostname("one.one.one.one"); // Cloudflare's DNS
EXPECT_FALSE(result.empty());
EXPECT_EQ(result, "1.1.1.1");
}
TEST(Tools, NetworkIPv6Exclusion)
{
// ipv6.google.com is guaranteed to have only IPv6 addresses
string result = fair::mq::tools::getIpFromHostname("ipv6.google.com");
EXPECT_TRUE(result.empty());
}
TEST(Tools, NetworkEmptyResult)
{
// Non-existent domain should return empty string
string result = fair::mq::tools::getIpFromHostname("non.existent.domain.invalid");
EXPECT_TRUE(result.empty());
}These tests verify:
Let me know if you'd like any modifications to these test cases. 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 68038 |
dennisklein
force-pushed
the
support-boost-1.87
branch
from
d869738 to
e3d71c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/tools/_network.cxx (1)
28-38: Consider making the tests more robust.The IPv4 resolution tests look good, but they could be enhanced:
- The Cloudflare DNS test depends on external service availability
- Consider adding timeout handling for network operations
Example enhancement:
TEST(Tools, NetworkIPv4Resolution) { string result = fair::mq::tools::getIpFromHostname("localhost"); EXPECT_FALSE(result.empty()); EXPECT_EQ(result, "127.0.0.1"); // Test a well-known public DNS that should resolve to an IPv4 + // TODO: Consider adding timeout handling result = fair::mq::tools::getIpFromHostname("one.one.one.one"); // Cloudflare's DNS EXPECT_FALSE(result.empty()); EXPECT_EQ(result, "1.1.1.1"); + + // Optional: Add fallback test in case external DNS is unavailable + if (result.empty()) { + GTEST_SKIP() << "External DNS test skipped - service unavailable"; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
fairmq/tools/Network.cxx(3 hunks)fairmq/tools/Process.cxx(3 hunks)test/tools/_network.cxx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- fairmq/tools/Network.cxx
- fairmq/tools/Process.cxx
🔇 Additional comments (3)
test/tools/_network.cxx (3)
2-2: LGTM! Proactive copyright year update.The copyright year range has been extended to 2025, which is good practice for actively maintained projects.
47-52: LGTM! Clear and focused test case.The empty result test is well-implemented and uses an appropriate invalid domain for testing.
Note: The
.invalidTLD is perfect for this test as it's specifically reserved for testing and will never resolve.
40-45: Consider using a more stable test domain for IPv6.While
ipv6.google.comworks, it's an external dependency that could change. Consider:
- Using a local IPv6-only hostname if available
- Adding a comment explaining why this specific domain was chosen
Let's verify if there are any local IPv6-only test domains we could use instead:
dennisklein
force-pushed
the
support-boost-1.87
branch
from
e3d71c9 to
b615fbe
Compare
dennisklein
force-pushed
the
support-boost-1.87
branch
from
b615fbe to
053a651
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cmake/FairMQProjectSettings.cmake (1)
191-193: Consider adding a comment explaining the timeout behavior.While the default value of 0 (no timeout) is a safe choice, it would be helpful to add a comment explaining:
- Why 0 was chosen as the default
- The implications of having no timeout
- Recommended values for different scenarios
fairmq/CMakeLists.txt (1)
2-2: Adjust copyright year to match current dateThe copyright year is set to 2025, which is beyond the current date (January 2025). Consider using 2024 instead, unless this is an intentional forward-looking update.
-# Copyright (C) 2012-2025 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH # +# Copyright (C) 2012-2024 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH #test/tools/_network.cxx (1)
42-46: Consider adding timeout verification tests.The tests for IPv6 exclusion and empty results are good, but consider adding tests to verify timeout behavior since the implementation in Network.cxx includes timeout handling.
Here's a suggested additional test case:
TEST(Tools, NetworkTimeout) { auto const start = std::chrono::steady_clock::now(); auto const ip = fair::mq::tools::getIpFromHostname("very.slow.domain.example"); auto const duration = std::chrono::steady_clock::now() - start; EXPECT_TRUE(ip.empty()); #if defined FAIRMQ_DNS_TIMEOUT_IN_MS && FAIRMQ_DNS_TIMEOUT_IN_MS > 0 EXPECT_LE(duration, std::chrono::milliseconds(FAIRMQ_DNS_TIMEOUT_IN_MS + 100)); // Add small buffer #endif }Also applies to: 48-52
fairmq/tools/Network.cxx (1)
162-195: Consider documenting the timeout behavior.The function's behavior has changed significantly with the async implementation and timeout handling. Consider adding a documentation comment to explain:
- The timeout behavior and configuration
- The empty string return cases (DNS error, timeout, IPv6-only)
Add a documentation comment:
+/** + * Resolves a hostname to its IPv4 address. + * + * @param hostname The hostname to resolve + * @return The IPv4 address as string, or empty string if: + * - DNS resolution fails + * - Only IPv6 addresses are available + * - Resolution times out (configurable via FAIRMQ_DNS_TIMEOUT_IN_MS) + */ string getIpFromHostname(const string& hostname)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
FairMQTest.cmake(2 hunks)cmake/FairMQProjectSettings.cmake(2 hunks)cmake/FairMQSummary.cmake(2 hunks)fairmq/CMakeLists.txt(2 hunks)fairmq/tools/Network.cxx(3 hunks)fairmq/tools/Process.cxx(3 hunks)test/CMakeLists.txt(1 hunks)test/tools/_network.cxx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fairmq/tools/Process.cxx
🧰 Additional context used
🪛 cppcheck (2.10-2)
test/tools/_network.cxx
[error] 28-28: syntax error
(syntaxError)
🔇 Additional comments (11)
FairMQTest.cmake (1)
44-49: Consider documenting the DNS timeout value choice.The test configuration sets
FAIRMQ_DNS_TIMEOUT_IN_MS=500, which differs from the default value of 0 inFairMQProjectSettings.cmake. This specific value choice for tests should be documented to explain why 500ms was chosen as an appropriate timeout for test scenarios.cmake/FairMQSummary.cmake (1)
102-102: LGTM! Clear and consistent status message.The status message for
FAIRMQ_DNS_TIMEOUT_IN_MSfollows the established pattern and clearly documents that 0 means no timeout.test/CMakeLists.txt (1)
256-256: Verify the impact of reducing the test timeout.The timeout for the Tools test suite has been reduced from 20 to 5 seconds, while other test suites maintain their 20-second timeout. Since this suite contains network-related tests:
- Ensure that 5 seconds is sufficient for DNS resolution tests with the new timeout mechanism
- Consider documenting why this specific test suite needs a different timeout
Run this script to check the test execution times:
fairmq/CMakeLists.txt (1)
177-177: Verify DNS timeout configurationThe addition of
FAIRMQ_DNS_TIMEOUT_IN_MSis crucial for network operations. Please ensure:
- A reasonable default value is set
- The timeout is appropriate for your use cases
- Documentation exists for this configuration option
Let's verify the configuration:
✅ Verification successful
DNS timeout configuration is properly implemented
The verification shows that:
- Default value of 0 (no timeout) is safe and reasonable
- Implementation in
Network.cxxcorrectly handles both timeout and no-timeout cases- Configuration is documented in CMake summary with clear explanation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the default value and documentation for FAIRMQ_DNS_TIMEOUT_IN_MS # Check for default value in CMake files echo "Checking default value..." rg "FAIRMQ_DNS_TIMEOUT_IN_MS" cmake/ # Check for documentation echo "Checking documentation..." fd -e md -e txt . | xargs rg "FAIRMQ_DNS_TIMEOUT_IN_MS"Length of output: 610
Script:
#!/bin/bash # Check for usage of DNS timeout in source code echo "Checking source code usage..." rg -A 2 "DNS_TIMEOUT" fairmq/Length of output: 815
test/tools/_network.cxx (4)
10-11: LGTM! Good addition of GMock for enhanced testing capabilities.The addition of GMock headers and using directives provides access to matchers like
AnyOfandStrEq, which are well utilized in the new test cases.Also applies to: 17-18
20-24: LGTM! Good use of type inference.The renaming of the test case to
NetworkDefaultIPbetter describes its purpose, and the use ofauto constimproves type safety while maintaining readability.
28-33: LGTM! Good test case for localhost resolution.The test case properly verifies both non-empty result and specific IPv4 address for localhost.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 28-28: syntax error
(syntaxError)
35-40: LGTM! Robust test for Cloudflare DNS resolution.Good use of
AnyOfmatcher to handle both possible valid IPv4 addresses (1.1.1.1 and 1.0.0.1) for Cloudflare's DNS.fairmq/tools/Network.cxx (3)
11-11: LGTM! Good addition of required headers.The addition of Strings.h and chrono headers supports the new implementation's needs.
Also applies to: 20-20
185-193: Good implementation of configurable timeout.The timeout implementation with the conditional compilation is well-documented and handles the limitations of Asio's DNS resolver implementation on Linux appropriately.
However, let's verify the timeout value usage across the codebase:
✅ Verification successful
DNS timeout implementation verified as correct and isolated
The timeout configuration
FAIRMQ_DNS_TIMEOUT_IN_MSis used consistently and exclusively for DNS resolution in Network.cxx, with proper documentation and fallback behavior. No conflicting timeout patterns were found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for FAIRMQ_DNS_TIMEOUT_IN_MS usage rg --type cpp "FAIRMQ_DNS_TIMEOUT_IN_MS" -B 2 -A 2 # Check for other timeout configurations rg --type cpp "run_for.*milliseconds" -B 2 -A 2Length of output: 1210
162-183: Verify error handling in the async callback.The async implementation looks good, but there are a few considerations:
- The error message uses
what()which might not provide the most descriptive message for DNS resolution errors- The empty result case logs a warning but doesn't distinguish between no results and IPv6-only results
Let's check how these error messages appear in practice:
Consider enhancing error messages:
- LOG(error) << "could not resolve hostname '" << hostname - << "', reason: " << ec.what(); + LOG(error) << "DNS resolution failed for '" << hostname + << "': " << ec.message() << " (code: " << ec.value() << ")";✅ Verification successful
Consider using
ec.message()instead ofec.what()for DNS resolution errorsWhile the IPv6 handling is working as intended (confirmed by test case), the error reporting can be improved:
- LOG(error) << "could not resolve hostname '" << hostname - << "', reason: " << ec.what(); + LOG(error) << "could not resolve hostname '" << hostname + << "', reason: " << ec.message() << " (code: " << ec.value() << ")";🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar error handling patterns in the codebase rg --type cpp "could not resolve hostname|could not find ipv4 address" -B 2 -A 2Length of output: 771
Script:
#!/bin/bash # Search for boost::asio resolver usage and error handling patterns ast-grep --pattern 'resolver.async_resolve($$$)' # Search for error_code handling patterns rg --type cpp "error_code.*what\(\)|error_code.*message\(\)" -B 2 -A 2 # Search for IPv6-related error handling rg --type cpp "ip::tcp::v6|ipv6" -B 2 -A 2Length of output: 1581
dennisklein
force-pushed
the
support-boost-1.87
branch
from
053a651 to
7665c93
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/tools/_network.cxx (1)
42-52: LGTM! Comprehensive error cases.Good coverage of edge cases:
- IPv6-only hostname returns empty string
- Non-existent domain returns empty string
However, consider adding a test for malformed hostnames to ensure robust error handling.
Would you like me to provide an example test case for malformed hostnames?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
FairMQTest.cmake(2 hunks)cmake/FairMQBundlePackageHelper.cmake(2 hunks)cmake/FairMQProjectSettings.cmake(2 hunks)cmake/FairMQSummary.cmake(2 hunks)fairmq/CMakeLists.txt(2 hunks)fairmq/Version.h.in(2 hunks)fairmq/tools/Network.cxx(3 hunks)fairmq/tools/Process.cxx(3 hunks)test/CMakeLists.txt(1 hunks)test/tools/_network.cxx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- fairmq/Version.h.in
🚧 Files skipped from review as they are similar to previous changes (7)
- fairmq/CMakeLists.txt
- FairMQTest.cmake
- test/CMakeLists.txt
- cmake/FairMQSummary.cmake
- cmake/FairMQProjectSettings.cmake
- fairmq/tools/Network.cxx
- fairmq/tools/Process.cxx
🧰 Additional context used
🪛 cppcheck (2.10-2)
test/tools/_network.cxx
[error] 28-28: syntax error
(syntaxError)
🔇 Additional comments (7)
cmake/FairMQBundlePackageHelper.cmake (2)
2-2: LGTM! Copyright year updated.The copyright year has been updated to include 2025, maintaining consistency across the project.
70-70: LGTM! Google Mock enabled.Enabling Google Mock (BUILD_GMOCK=ON) aligns with the new test cases that utilize Google Mock features.
test/tools/_network.cxx (5)
2-2: LGTM! Copyright year updated.The copyright year has been updated to include 2025, maintaining consistency across the project.
10-18: LGTM! Google Mock integration.Good addition of Google Mock headers and using directives, which improve readability and align with the enabled BUILD_GMOCK option.
20-26: LGTM! Enhanced test clarity.Good improvements:
- Test renamed to NetworkDefaultIP for better clarity
- Using auto const for type inference
28-33: Verify localhost resolution behavior across environments.While testing localhost resolution is good, be aware that some environments might have different localhost configurations.
Consider adding a comment explaining the expected behavior and any environment-specific considerations.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 28-28: syntax error
(syntaxError)
35-40: LGTM! Robust Cloudflare DNS test.Good use of Google Mock's AnyOf matcher to handle both possible Cloudflare DNS IPs (1.1.1.1 and 1.0.0.1).
dennisklein
force-pushed
the
support-boost-1.87
branch
from
7665c93 to
6c002a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/tools/_network.cxx (1)
28-33: Consider adding error message for IP validation.While the test case correctly validates localhost resolution, consider adding a descriptive error message to
EXPECT_FALSEfor better debugging.- EXPECT_FALSE(ip.empty()); + EXPECT_FALSE(ip.empty()) << "Expected localhost to resolve to a valid IP";🧰 Tools
🪛 cppcheck (2.10-2)
[error] 28-28: syntax error
(syntaxError)
fairmq/tools/Network.cxx (1)
195-195: Consider error propagation.The function returns an empty string for both successful empty results and error conditions. Consider using
std::optionalor a custom result type to distinguish between these cases.struct ResolveResult { string ip; enum class Status { Success, Empty, Error } status; string error_message; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
FairMQTest.cmake(2 hunks)cmake/FairMQBundlePackageHelper.cmake(2 hunks)cmake/FairMQProjectSettings.cmake(2 hunks)cmake/FairMQSummary.cmake(2 hunks)fairmq/CMakeLists.txt(2 hunks)fairmq/Version.h.in(2 hunks)fairmq/tools/Network.cxx(3 hunks)fairmq/tools/Process.cxx(3 hunks)test/CMakeLists.txt(1 hunks)test/tools/_network.cxx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- fairmq/CMakeLists.txt
- test/CMakeLists.txt
- cmake/FairMQProjectSettings.cmake
- cmake/FairMQBundlePackageHelper.cmake
- FairMQTest.cmake
- fairmq/Version.h.in
- fairmq/tools/Process.cxx
- cmake/FairMQSummary.cmake
🧰 Additional context used
🪛 cppcheck (2.10-2)
test/tools/_network.cxx
[error] 28-28: syntax error
(syntaxError)
🔇 Additional comments (5)
test/tools/_network.cxx (4)
10-11: LGTM! Good addition of GMock framework.The inclusion of GMock and its matchers (
AnyOf,StrEq) enhances the test assertions capabilities.Also applies to: 17-18
20-24: LGTM! Clear test case renaming.The test case renaming from
NetworktoNetworkDefaultIPbetter reflects its purpose. The use ofauto constfor variable declarations is a good practice for type safety and immutability.
42-46: LGTM! Good negative test cases.The test cases for IPv6 exclusion and empty results are well-designed:
- IPv6 exclusion ensures the function correctly handles IPv6-only hosts
- Empty result test verifies proper handling of non-existent domains
Also applies to: 48-52
35-40: Verify Cloudflare DNS stability for CI.The test relies on external Cloudflare DNS. While it's a reliable service, consider:
- Adding a timeout to prevent test hangs
- Adding a comment explaining why these specific IPs are expected
- Consider mocking DNS resolution for more reliable CI
✅ Verification successful
Timeout mechanism exists, documentation could be improved
The implementation already includes a configurable timeout via
FAIRMQ_DNS_TIMEOUT_IN_MS. Consider:
- Adding a comment in the test explaining that 1.1.1.1 and 1.0.0.1 are Cloudflare's primary DNS servers
- Documenting the timeout configuration in the test file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if Cloudflare DNS IPs are consistently reachable from CI # Test: Check if Cloudflare DNS is reachable ping -c 1 -W 2 one.one.one.one > /dev/null if [ $? -eq 0 ]; then echo "Cloudflare DNS is reachable" else echo "Warning: Cloudflare DNS might be unreachable in CI" fiLength of output: 258
Script:
#!/bin/bash # Find implementation of getIpFromHostname ast-grep --pattern 'getIpFromHostname' -A 10 # Search for any documentation about DNS resolution rg -i "dns|cloudflare|one\.one\.one\.one" --type md # Look for timeout related configurations rg -i "timeout.*getip|getip.*timeout"Length of output: 3145
Script:
#!/bin/bash # Get the complete implementation of getIpFromHostname cat fairmq/tools/Network.cxx # Look for io_context configurations rg -i "io_context.*run|run.*timeout" fairmq/tools/Network.cxxLength of output: 6393
fairmq/tools/Network.cxx (1)
185-193: Good timeout handling with conditional compilation.The timeout handling is well-implemented with:
- Configurable timeout via
FAIRMQ_DNS_TIMEOUT_IN_MS- Fallback to blocking behavior when timeout is disabled
- Good documentation of getaddrinfo limitations
Deprecated via chriskohlhoff/asio@74fe2b8 and removed via chriskohlhoff/asio@e916bdf in Boost 1.87 or Asio 1.33.
Deprecated via chriskohlhoff/asio@d3bbf37 and removed via chriskohlhoff/asio@49fcd03 in Boost 1.87 or Asio 1.33.
dennisklein
force-pushed
the
support-boost-1.87
branch
from
6c002a3 to
d35a58d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fairmq/tools/Network.cxx (1)
161-176: Consider asynchronous resolution for better performance.The current synchronous implementation may block the calling thread. Consider using asynchronous resolution with a future-based approach for better performance in high-concurrency scenarios.
Here's a non-blocking alternative:
-string getIpFromHostname(const string& hostname) +future<string> getIpFromHostname(const string& hostname) try { boost::asio::io_context ioc; boost::asio::ip::tcp::resolver resolver(ioc); - auto const result = resolver.resolve(boost::asio::ip::tcp::v4(), hostname, ""); - if (result.empty()) { - LOG(warn) << "could not find ipv4 address for hostname '" << hostname << "'"; - return ""; - } - return ToString(result.begin()->endpoint().address()); + promise<string> result_promise; + auto result_future = result_promise.get_future(); + + resolver.async_resolve( + boost::asio::ip::tcp::v4(), + hostname, + "", + [promise = move(result_promise)] + (const boost::system::error_code& ec, const resolver_results& result) mutable { + if (ec || result.empty()) { + LOG(warn) << "could not find ipv4 address for hostname '" << hostname << "'"; + promise.set_value(""); + return; + } + promise.set_value(ToString(result.begin()->endpoint().address())); + } + ); + + ioc.run(); + return result_future; } catch (std::exception const& ex) { LOG(error) << "could not resolve hostname '" << hostname << "', reason: " << ex.what(); - return ""; + return make_ready_future<string>(""); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
fairmq/Version.h.in(2 hunks)fairmq/tools/Network.cxx(3 hunks)fairmq/tools/Process.cxx(3 hunks)test/CMakeLists.txt(1 hunks)test/tools/_network.cxx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/CMakeLists.txt
- fairmq/Version.h.in
- fairmq/tools/Process.cxx
🧰 Additional context used
🪛 cppcheck (2.10-2)
test/tools/_network.cxx
[error] 24-24: syntax error
(syntaxError)
🔇 Additional comments (5)
test/tools/_network.cxx (3)
16-22: LGTM! Clear and descriptive test case renaming.The rename from
NetworktoNetworkDefaultIPbetter describes the test's purpose.
24-29: LGTM! Well-structured localhost resolution test.The test properly verifies both non-empty result and specific IPv4 address resolution.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 24-24: syntax error
(syntaxError)
31-35: LGTM! Good error case coverage.The test ensures graceful handling of invalid hostnames.
fairmq/tools/Network.cxx (2)
161-172: LGTM! Thread-safe implementation.The synchronous implementation resolves the thread safety concerns raised in the previous review by eliminating shared state.
173-176: LGTM! Proper error handling.Good error handling with appropriate logging of the exception details.
|
@rbx Tried to implement a timeout for the dns resolver, led to tons of more code, just to find out, it is not effective as Asio uses blocking OS APIs. So, in the final version of this PR I reverted back to the original behaviour which relies on system timeouts and has survived until today anyways. |
|
One of the tests fails here without any output: https://cdash.gsi.de/testDetails.php?test=22562095&build=488038 |
Ran it again two times, seems good now. We'll see if this test turns out to be unstable. |
2 participants
No description provided.