[Refactor] Network Improvements - #244
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds mDNS lifecycle control and robust mDNS resolution, a NetworkUtils module for IP parsing/selection, persists mdns_instance_name in host state with deduplication, and wires suspend/resume plus navigation hooks to teardown/reinitialize MDNS. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Lifecycle
participant MDNS as MDNSHandler
participant State as ApplicationState
App->>MDNS: close_mdns() on suspend
MDNS->>MDNS: close sockets, cleanup state
App->>MDNS: reinit_mdns() on resume
MDNS->>MDNS: reopen sockets, enable multicast, broadcast PTR
MDNS->>State: UpdateHostAddressForInstance(mdnsInstanceName, host:port)
State->>State: deduplicate & persist host (mdns_instance_name)
sequenceDiagram
participant Page as HostSelectorPage
participant Utils as NetworkUtils
participant MDNS as MDNSHandler
participant State as ApplicationState
Page->>MDNS: init_mdns() on navigate
MDNS->>MDNS: send PTR/SRV queries, receive A/AAAA
MDNS->>MDNS: map instance->host, host->ips
MDNS->>State: UpdateHostAddressForInstance(mdnsInstanceName, addr)
State->>Utils: SplitIPAddress(host:port)
Utils->>State: return ip and port
State->>State: update/persist host record
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Pages/HostSelectorPage.xaml.cpp (1)
270-278: Potential socket leak on repeated page navigation.
init_mdns()is called each timeOnNavigatedTofires, but sockets from a previous navigation are never closed (the page doesn't callclose_mdns()on exit). The staticsockets[]array will be overwritten, leaking previously opened sockets.Consider calling
close_mdns()inOnNavigatedFrom, or usereinit_mdns()here instead ofinit_mdns()to ensure stale sockets are cleaned up before opening new ones.Suggested fix: use reinit_mdns() or add cleanup
void HostSelectorPage::OnNavigatedTo(Windows::UI::Xaml::Navigation::NavigationEventArgs^ e) { Windows::UI::ViewManagement::ApplicationView::GetForCurrentView()->SetDesiredBoundsMode(Windows::UI::ViewManagement::ApplicationViewBoundsMode::UseVisible); continueFetch.store(true); Concurrency::create_task([this] { - init_mdns(); + reinit_mdns(); // Ensures previous sockets are closed before opening new ones while (continueFetch.load()) {Or add an
OnNavigatedFromhandler:void HostSelectorPage::OnNavigatedFrom(Windows::UI::Xaml::Navigation::NavigationEventArgs^ e) { continueFetch.store(false); close_mdns(); }
🤖 Fix all issues with AI agents
In @App.xaml.cpp:
- Around line 105-108: Currently exceptions from close_mdns() in App.xaml.cpp
are swallowed, hiding teardown errors; update the try/catch to catch
std::exception and any other exceptions and log them via the existing logging
facility (e.g., processLogger or app logger) including the exception.what() for
std::exception and a generic message for unknown exceptions, so change the empty
catch(...) to one that logs the error and retains the swallow behavior to avoid
suspend failures while surfacing diagnostics for close_mdns().
- Around line 121-124: The try/catch around reinit_mdns() currently swallows all
exceptions; update it to catch std::exception and a fallback catch(...) and log
the failure so mDNS reinit errors are visible—call the project's logging
facility (or OutputDebug/Trace) from the catch(std::exception& e) with a message
like "reinit_mdns failed: " + e.what(), and in catch(...) log an "unknown
exception in reinit_mdns" message; keep calling reinit_mdns() in the try block
and ensure the logs include enough context to diagnose resume/discovery
failures.
In @State/ApplicationState.cpp:
- Around line 348-363: Wrap the calls to NetworkUtils::SplitIPAddress and
NetworkUtils::GetBroadcastIP in try-catch blocks to prevent exceptions from
propagating when LastHostname or ServerAddress are malformed; catch
std::invalid_argument and std::out_of_range (or std::exception as a fallback),
log an informative message via Utils::Log (including the offending
LastHostname/ServerAddress) and skip pushing results into the addresses vector
so the WoL flow continues safely. Ensure the try-catch surrounds the
SplitIPAddress call where host->LastHostname is used and the GetBroadcastIP call
where host->ServerAddress is used, and do not rethrow the exceptions.
- Around line 102-115: The race occurs because UpdateFile() is called
immediately after dispatching the UI thread lambda, so SavedHosts->Append(host)
(inside the RunAsync lambda) may not have run yet; fix by invoking UpdateFile()
from inside the dispatched handler immediately after SavedHosts->Append(host)
(i.e., move the UpdateFile() call into the lambda that runs in
Windows::UI::Core::DispatchedHandler) so the file update happens only after the
host is appended (alternatively capture the IAsyncAction returned by RunAsync
and wait for completion before calling UpdateFile(), but the simplest fix is to
call UpdateFile() right after SavedHosts->Append(host) within the dispatched
callback).
- Line 186: hostJson["mdns_instance_name"] is set using
Utils::PlatformStringToStdString(host->MdnsInstanceName) without a nullptr
check; change this to guard against host->MdnsInstanceName being nullptr (the
same pattern used elsewhere) by using a conditional expression so that if
host->MdnsInstanceName is nullptr you assign an empty string, otherwise call
Utils::PlatformStringToStdString(host->MdnsInstanceName), keeping the assignment
to hostJson["mdns_instance_name"] unchanged.
In @State/MDNSHandler.cpp:
- Around line 27-34: In ipv4_address_to_string, the local variable named
"service" shadows the file-scope "service" constant; rename the local to
something distinct (e.g., service_buf or service_local) and update all uses
within ipv4_address_to_string (including the getnameinfo call and any subsequent
references) to avoid the shadowing while preserving the buffer size NI_MAXSERV.
In @Utils/NetworkUtils.cpp:
- Around line 154-157: The GetBroadcastIP function incorrectly calls
WSACleanup() on its error path, which will decrement Winsock's reference count
even though GetBroadcastIP does not call WSAStartup(); remove the errant
WSACleanup() invocation from GetBroadcastIP (and any similar early-return error
branches) so Winsock cleanup is only performed by the code that called
WSAStartup() or a dedicated shutdown routine; if necessary, ensure any
WSAStartup() caller is responsible for a matching WSACleanup() instead of doing
cleanup inside GetBroadcastIP.
🧹 Nitpick comments (3)
State/MDNSHandler.cpp (1)
152-216: Consider using NetworkUtils to reduce duplication.These static IP classification and priority functions (
is_ipv4,is_private_ipv4,ip_priority,pick_best_ip) duplicate the logic inNetworkUtils. Consider includingNetworkUtils.hand calling the shared implementations to maintain consistency and reduce maintenance burden.Utils/NetworkUtils.h (1)
1-25: LGTM!Clean header with well-organized utility function declarations. The API provides comprehensive IP address classification and manipulation capabilities.
Minor nit: Line 23 uses different indentation than the rest of the file.
Utils/NetworkUtils.cpp (1)
8-8: Unused include:nlohmann/json.hpp.This header is included but not used anywhere in the file. Remove it to reduce compilation time and dependencies.
-#include <nlohmann/json.hpp>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
App.xaml.cppPackage.appxmanifestPages/HostSelectorPage.xaml.cppState/ApplicationState.cppState/ApplicationState.hState/MDNSHandler.cppState/MDNSHandler.hState/MoonlightHost.hUtils/NetworkUtils.cppUtils/NetworkUtils.hmoonlight-xbox-dx.vcxprojmoonlight-xbox-dx.vcxproj.filters
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-01-08T15:15:34.393Z
Learnt from: mpotrykus
Repo: TheElixZammuto/moonlight-xbox PR: 243
File: moonlight-xbox-dx.vcxproj:661-663
Timestamp: 2026-01-08T15:15:34.393Z
Learning: In the moonlight-xbox project (moonlight-xbox-dx.vcxproj), the ProjectReference to third_party\DirectXTK\DirectXTK_Windows10_2022.vcxproj is intentionally added before the file exists. The setup-dev.ps1 script generates this project file during setup, and the reference ensures the correct build order (DirectXTK before main project) once everything is generated, avoiding manual build requirements.
Applied to files:
moonlight-xbox-dx.vcxproj.filtersmoonlight-xbox-dx.vcxproj
📚 Learning: 2025-02-27T02:36:15.388Z
Learnt from: LAGonauta
Repo: TheElixZammuto/moonlight-xbox PR: 167
File: State/MoonlightHost.h:38-38
Timestamp: 2025-02-27T02:36:15.388Z
Learning: In MoonlightHost class, hdmiDisplayMode should be explicitly initialized to nullptr in the constructor, and null checks should be added before accessing it in methods like Connect(). This is particularly important because hdmiDisplayMode might not be initialized when loading previously saved hosts that don't contain display information.
Applied to files:
State/ApplicationState.cpp
📚 Learning: 2025-02-27T02:36:15.388Z
Learnt from: LAGonauta
Repo: TheElixZammuto/moonlight-xbox PR: 167
File: State/MoonlightHost.h:38-38
Timestamp: 2025-02-27T02:36:15.388Z
Learning: When using hdmiDisplayMode in the MoonlightHost::Connect() method, a null check should be performed since hdmiDisplayMode might be null when loading previously saved hosts without display information. The MoonlightClient constructor should either handle null parameters or the Connect() method should initialize hdmiDisplayMode with a default value before passing it.
Applied to files:
State/ApplicationState.cpp
📚 Learning: 2025-02-12T16:34:25.238Z
Learnt from: mpotrykus
Repo: TheElixZammuto/moonlight-xbox PR: 163
File: State/ApplicationState.cpp:0-0
Timestamp: 2025-02-12T16:34:25.238Z
Learning: Wake-on-LAN magic packets must be sent to either port 7 or port 9 on the destination host as per the protocol specification. The source port can be ephemeral but the destination port must be either 7 or 9.
Applied to files:
State/ApplicationState.cpp
🧬 Code graph analysis (5)
State/MDNSHandler.h (1)
State/MDNSHandler.cpp (6)
query_mdns(417-472)query_mdns(417-417)close_mdns(474-483)close_mdns(474-474)reinit_mdns(485-492)reinit_mdns(485-485)
Utils/NetworkUtils.h (1)
Utils/NetworkUtils.cpp (22)
ExtractIpFromHostPort(12-30)ExtractIpFromHostPort(12-12)IsIpv4(32-34)IsIpv4(32-32)IsIpv6(36-38)IsIpv6(36-36)IsPrivateIpv4(40-54)IsPrivateIpv4(40-40)IsIpv6Ula(56-61)IsIpv6Ula(56-56)IsIpv6Gua(67-71)IsIpv6Gua(67-67)IsIpv6LinkLocal(63-65)IsIpv6LinkLocal(63-63)IpPriority(74-88)IpPriority(74-74)PickBestIp(90-103)PickBestIp(90-90)SplitIPAddress(105-123)SplitIPAddress(105-105)GetBroadcastIP(125-170)GetBroadcastIP(125-125)
State/ApplicationState.cpp (4)
Common/ModalDialog.xaml.cpp (16)
a(157-159)a(159-161)a(163-165)a(165-167)a(167-169)a(205-205)a(221-221)a(226-228)a(228-230)a(261-261)a(277-277)a(282-284)a(284-286)a(296-296)a(360-362)a(362-364)Utils.cpp (6)
Logf(90-99)Logf(90-90)Log(57-82)Log(57-57)Log(84-88)Log(84-84)Utils.hpp (3)
Logf(15-15)Log(13-13)Log(14-14)Utils/NetworkUtils.cpp (8)
ExtractIpFromHostPort(12-30)ExtractIpFromHostPort(12-12)IpPriority(74-88)IpPriority(74-74)SplitIPAddress(105-123)SplitIPAddress(105-105)GetBroadcastIP(125-170)GetBroadcastIP(125-125)
State/MDNSHandler.cpp (2)
State/ApplicationState.h (1)
moonlight_xbox_dx(5-148)State/MoonlightHost.h (1)
moonlight_xbox_dx(5-297)
App.xaml.cpp (1)
State/MDNSHandler.cpp (4)
close_mdns(474-483)close_mdns(474-474)reinit_mdns(485-492)reinit_mdns(485-485)
🔇 Additional comments (21)
Package.appxmanifest (1)
49-50: Verify the necessity of theinternetClientServercapability.The PR introduces mDNS-based host discovery, which typically operates on the local network. The existing
privateNetworkClientServercapability should be sufficient for local network mDNS operations. TheinternetClientServercapability grants broader internet access (both inbound and outbound), which may not be required unless the mDNS implementation needs to operate beyond the local network or supports internet-based discovery scenarios.Can you confirm whether internet-level access is required for the mDNS implementation, or would
privateNetworkClientServeralone suffice for local network discovery?moonlight-xbox-dx.vcxproj.filters (1)
479-481: LGTM!The addition of
Utils\NetworkUtils.hto the Header Files filter is consistent with the new network utilities module introduced in this PR.moonlight-xbox-dx.vcxproj (2)
385-385: LGTM!The addition of
Utils\NetworkUtils.hto the build is correct and follows the existing project structure.
560-560: LGTM!The addition of
Utils\NetworkUtils.cppto the build is correct and completes the integration of the NetworkUtils module.App.xaml.cpp (1)
9-9: LGTM!The inclusion of
State/MDNSHandler.his necessary to support mDNS lifecycle management during app suspend/resume events.State/MoonlightHost.h (1)
12-12: LGTM!The addition of the
MdnsInstanceNameproperty follows the established pattern used by other properties in the class. The implementation correctly includes the private field, getter, setter, and property change notification for data binding support.Also applies to: 55-62
State/MDNSHandler.h (1)
4-7: LGTM! Clean lifecycle API for mDNS management.The new
close_mdns()andreinit_mdns()functions provide proper lifecycle hooks for suspend/resume scenarios.Pages/HostSelectorPage.xaml.cpp (3)
12-12: LGTM!Include path for NetworkUtils is correctly added.
63-63: LGTM!Passing
nullptrformdnsInstanceNameis appropriate for manually-added hosts.
405-415: LGTM!Good defensive fallback pattern for IP parsing. The catch-all gracefully handles malformed input.
State/ApplicationState.h (1)
20-21: LGTM!Clean API additions for mDNS-based host management:
AddHostnow accepts an optionalmdnsInstanceNamefor deduplication.UpdateHostAddressForInstanceenables silent IP upgrades based on priority.State/ApplicationState.cpp (2)
34-35: LGTM!Correctly loading
mdns_instance_namefrom persisted state and assigning to the newMdnsInstanceNameproperty.
118-165: LGTM!The
UpdateHostAddressForInstanceimplementation correctly:
- Validates input parameters
- Matches hosts by
MdnsInstanceName- Prevents IP regression using priority scoring
- Persists changes via
UpdateFile()State/MDNSHandler.cpp (5)
16-18: LGTM!The state maps (
instanceToHost,instanceToPort,hostToIps) provide a clean structure for accumulating mDNS resolution data across multiple callbacks.
84-150: LGTM!The
dns_read_namefunction correctly handles DNS name compression with proper bounds checking and null-termination. The jump offset tracking for compression pointers is correctly implemented.
314-365: LGTM!The resolution chain logic correctly:
- Validates complete instance → hostname → IP → port mappings before proceeding
- Selects the best IP using priority scoring
- Properly formats IPv6 addresses with brackets
- Attempts silent upgrade via
UpdateHostAddressForInstancebefore adding new hosts- Safely modifies maps while iterating using iterator-based erase
377-398: IPv6 mDNS sockets not opened; discovery limited to IPv4 interfaces.The PR description mentions IPv6 support, but
init_mdns()only opens IPv4 sockets (line 379 checksHostNameType::Ipv4). While AAAA records are parsed from responses, mDNS queries are only sent over IPv4 interfaces. This means hosts on IPv6-only networks may not be discovered.Consider adding IPv6 socket support using
mdns_socket_open_ipv6()if full IPv6 mDNS discovery is intended.
474-492: LGTM!Lifecycle functions properly manage socket cleanup and state reset.
reinit_mdns()correctly clears discovery state maps before re-initialization.Utils/NetworkUtils.cpp (3)
12-30: LGTM!Correctly handles both IPv4 (
ip:port) and IPv6 ([ip]:port) formats with safe fallbacks for malformed input.
32-71: LGTM!IP classification functions are sufficient for the use case of prioritizing connection addresses. Note:
IsPrivateIpv4doesn't include link-local (169.254.x.x) or loopback (127.x.x.x), which is appropriate since those shouldn't be used for host connections anyway.
105-123:SplitIPAddresswith:delimiter doesn't handle IPv6 addresses correctly.Two callers (HostSelectorPage.xaml.cpp:406 and ApplicationState.cpp:350) use
:as a delimiter to split hostname and port. However, IPv6 addresses contain colons (e.g.,::1:8080or2001:db8::1:443), so this approach fails for IPv6. The function itself is correctly parameterized, but if these callers might receive IPv6 addresses, they need special handling (typically with brackets:[::1]:8080). Clarify whether these callers are IPv4-only, or add IPv6 support.
|
|
||
| try { | ||
| close_mdns(); | ||
| } catch(...) {} |
There was a problem hiding this comment.
Add logging for mDNS teardown failures.
Silently swallowing all exceptions during mDNS teardown masks potential errors that could help diagnose issues. While it's appropriate to prevent suspend failures due to mDNS cleanup problems, logging the exception would provide valuable diagnostic information.
📝 Suggested improvement
try {
close_mdns();
- } catch(...) {}
+ } catch(...) {
+ Utils::Log("[App] Failed to close mDNS during suspend\n");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| close_mdns(); | |
| } catch(...) {} | |
| try { | |
| close_mdns(); | |
| } catch(...) { | |
| Utils::Log("[App] Failed to close mDNS during suspend\n"); | |
| } |
🤖 Prompt for AI Agents
In @App.xaml.cpp around lines 105 - 108, Currently exceptions from close_mdns()
in App.xaml.cpp are swallowed, hiding teardown errors; update the try/catch to
catch std::exception and any other exceptions and log them via the existing
logging facility (e.g., processLogger or app logger) including the
exception.what() for std::exception and a generic message for unknown
exceptions, so change the empty catch(...) to one that logs the error and
retains the swallow behavior to avoid suspend failures while surfacing
diagnostics for close_mdns().
|
|
||
| try { | ||
| reinit_mdns(); | ||
| } catch(...) {} |
There was a problem hiding this comment.
Add logging for mDNS reinitialization failures.
Silently swallowing all exceptions during mDNS reinitialization masks potential errors. If reinit_mdns() fails, the app will continue but host discovery will be broken with no indication to the user or in logs. Adding error logging would help diagnose post-resume discovery issues.
📝 Suggested improvement
try {
reinit_mdns();
- } catch(...) {}
+ } catch(...) {
+ Utils::Log("[App] Failed to reinitialize mDNS during resume\n");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| reinit_mdns(); | |
| } catch(...) {} | |
| try { | |
| reinit_mdns(); | |
| } catch(...) { | |
| Utils::Log("[App] Failed to reinitialize mDNS during resume\n"); | |
| } |
🤖 Prompt for AI Agents
In @App.xaml.cpp around lines 121 - 124, The try/catch around reinit_mdns()
currently swallows all exceptions; update it to catch std::exception and a
fallback catch(...) and log the failure so mDNS reinit errors are visible—call
the project's logging facility (or OutputDebug/Trace) from the
catch(std::exception& e) with a message like "reinit_mdns failed: " + e.what(),
and in catch(...) log an "unknown exception in reinit_mdns" message; keep
calling reinit_mdns() in the try block and ensure the logs include enough
context to diagnose resume/discovery failures.
| Windows::ApplicationModel::Core::CoreApplication::MainView->CoreWindow->Dispatcher->RunAsync( | ||
| Windows::UI::Core::CoreDispatcherPriority::High, | ||
| ref new Windows::UI::Core::DispatchedHandler([this, host]() { | ||
|
|
||
| Utils::Logf("[AddHost] NEW HOST: InstanceId='%s' MdnsInstance='%s' LastHostname='%s'\n", | ||
| Utils::PlatformStringToStdString(host->InstanceId).c_str(), | ||
| host->MdnsInstanceName ? Utils::PlatformStringToStdString(host->MdnsInstanceName).c_str() : "(null)", | ||
| Utils::PlatformStringToStdString(host->LastHostname).c_str()); | ||
|
|
||
| SavedHosts->Append(host); | ||
| })); | ||
|
|
||
| UpdateFile(); | ||
| return true; |
There was a problem hiding this comment.
Race condition: UpdateFile() may execute before host is appended.
UpdateFile() is called immediately after RunAsync dispatches the append operation. Since RunAsync is asynchronous, SavedHosts->Append(host) may not complete before UpdateFile() iterates over SavedHosts, causing the new host to be missed in the persisted state.
Suggested fix: Move UpdateFile inside the dispatcher callback
Windows::ApplicationModel::Core::CoreApplication::MainView->CoreWindow->Dispatcher->RunAsync(
Windows::UI::Core::CoreDispatcherPriority::High,
ref new Windows::UI::Core::DispatchedHandler([this, host]() {
Utils::Logf("[AddHost] NEW HOST: InstanceId='%s' MdnsInstance='%s' LastHostname='%s'\n",
Utils::PlatformStringToStdString(host->InstanceId).c_str(),
host->MdnsInstanceName ? Utils::PlatformStringToStdString(host->MdnsInstanceName).c_str() : "(null)",
Utils::PlatformStringToStdString(host->LastHostname).c_str());
SavedHosts->Append(host);
+ UpdateFile();
}));
- UpdateFile();
return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Windows::ApplicationModel::Core::CoreApplication::MainView->CoreWindow->Dispatcher->RunAsync( | |
| Windows::UI::Core::CoreDispatcherPriority::High, | |
| ref new Windows::UI::Core::DispatchedHandler([this, host]() { | |
| Utils::Logf("[AddHost] NEW HOST: InstanceId='%s' MdnsInstance='%s' LastHostname='%s'\n", | |
| Utils::PlatformStringToStdString(host->InstanceId).c_str(), | |
| host->MdnsInstanceName ? Utils::PlatformStringToStdString(host->MdnsInstanceName).c_str() : "(null)", | |
| Utils::PlatformStringToStdString(host->LastHostname).c_str()); | |
| SavedHosts->Append(host); | |
| })); | |
| UpdateFile(); | |
| return true; | |
| Windows::ApplicationModel::Core::CoreApplication::MainView->CoreWindow->Dispatcher->RunAsync( | |
| Windows::UI::Core::CoreDispatcherPriority::High, | |
| ref new Windows::UI::Core::DispatchedHandler([this, host]() { | |
| Utils::Logf("[AddHost] NEW HOST: InstanceId='%s' MdnsInstance='%s' LastHostname='%s'\n", | |
| Utils::PlatformStringToStdString(host->InstanceId).c_str(), | |
| host->MdnsInstanceName ? Utils::PlatformStringToStdString(host->MdnsInstanceName).c_str() : "(null)", | |
| Utils::PlatformStringToStdString(host->LastHostname).c_str()); | |
| SavedHosts->Append(host); | |
| UpdateFile(); | |
| })); | |
| return true; |
🤖 Prompt for AI Agents
In @State/ApplicationState.cpp around lines 102 - 115, The race occurs because
UpdateFile() is called immediately after dispatching the UI thread lambda, so
SavedHosts->Append(host) (inside the RunAsync lambda) may not have run yet; fix
by invoking UpdateFile() from inside the dispatched handler immediately after
SavedHosts->Append(host) (i.e., move the UpdateFile() call into the lambda that
runs in Windows::UI::Core::DispatchedHandler) so the file update happens only
after the host is appended (alternatively capture the IAsyncAction returned by
RunAsync and wait for completion before calling UpdateFile(), but the simplest
fix is to call UpdateFile() right after SavedHosts->Append(host) within the
dispatched callback).
| nlohmann::json hostJson; | ||
| hostJson["hostname"] = Utils::PlatformStringToStdString(host->LastHostname); | ||
| hostJson["instance_id"] = Utils::PlatformStringToStdString(host->InstanceId); | ||
| hostJson["mdns_instance_name"] = Utils::PlatformStringToStdString(host->MdnsInstanceName); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nA10 "PlatformStringToStdString" --type=cpp | head -100Repository: TheElixZammuto/moonlight-xbox
Length of output: 5888
🏁 Script executed:
sed -n '180,195p' State/ApplicationState.cpp | cat -nRepository: TheElixZammuto/moonlight-xbox
Length of output: 1206
🏁 Script executed:
# Check host definition and MdnsInstanceName initialization
rg -nB3 "MdnsInstanceName" State/ApplicationState.cpp | head -50Repository: TheElixZammuto/moonlight-xbox
Length of output: 2181
Add null-check before converting host->MdnsInstanceName to string.
Line 186 calls Utils::PlatformStringToStdString(host->MdnsInstanceName) without checking for nullptr. The PlatformStringToStdString implementation (Utils.cpp:117-119) does not handle nullptr input and will crash. Since MdnsInstanceName can be nullptr for manually-added hosts (see lines 81-82, 108, and 131 which already guard against this), line 186 must use the same pattern: host->MdnsInstanceName != nullptr ? Utils::PlatformStringToStdString(host->MdnsInstanceName) : "".
🤖 Prompt for AI Agents
In @State/ApplicationState.cpp at line 186, hostJson["mdns_instance_name"] is
set using Utils::PlatformStringToStdString(host->MdnsInstanceName) without a
nullptr check; change this to guard against host->MdnsInstanceName being nullptr
(the same pattern used elsewhere) by using a conditional expression so that if
host->MdnsInstanceName is nullptr you assign an empty string, otherwise call
Utils::PlatformStringToStdString(host->MdnsInstanceName), keeping the assignment
to hostJson["mdns_instance_name"] unchanged.
| if (host->LastHostname) | ||
| { | ||
| auto hostnameSplit = Split_IP_Address(Utils::PlatformStringToStdString(host->LastHostname), ':'); | ||
| auto hostnameSplit = NetworkUtils::SplitIPAddress(Utils::PlatformStringToStdString(host->LastHostname), ':'); | ||
| std::string hostIp = hostnameSplit.first; | ||
| addresses.push_back(hostIp); | ||
| } | ||
|
|
||
| if (inet_addr(Utils::PlatformStringToStdString(host->ServerAddress).c_str()) != -1) | ||
| { | ||
| std::string broadcastIP = Get_Broadcast_IP(Utils::PlatformStringToStdString(host->ServerAddress)); | ||
| if (broadcastIP != "") | ||
| std::string broadcastIP = NetworkUtils::GetBroadcastIP(Utils::PlatformStringToStdString(host->ServerAddress)); | ||
| if (broadcastIP != "") | ||
| { | ||
| addresses.push_back(broadcastIP); | ||
| } else { | ||
| Utils::Log("Could not determine subnet mask from IP address.\n"); | ||
| } |
There was a problem hiding this comment.
Missing exception handling for SplitIPAddress and GetBroadcastIP.
Both NetworkUtils::SplitIPAddress (line 350) and NetworkUtils::GetBroadcastIP (line 357) can throw exceptions (std::invalid_argument, std::out_of_range). If LastHostname or ServerAddress contains malformed data, this will propagate up and potentially crash the WoL flow.
Suggested fix: Wrap in try-catch
if (host->LastHostname)
{
+ try {
auto hostnameSplit = NetworkUtils::SplitIPAddress(Utils::PlatformStringToStdString(host->LastHostname), ':');
std::string hostIp = hostnameSplit.first;
addresses.push_back(hostIp);
+ } catch (...) {
+ Utils::Log("Failed to parse LastHostname for WoL\n");
+ }
}
if (inet_addr(Utils::PlatformStringToStdString(host->ServerAddress).c_str()) != -1)
{
- std::string broadcastIP = NetworkUtils::GetBroadcastIP(Utils::PlatformStringToStdString(host->ServerAddress));
- if (broadcastIP != "")
- {
- addresses.push_back(broadcastIP);
- } else {
- Utils::Log("Could not determine subnet mask from IP address.\n");
+ try {
+ std::string broadcastIP = NetworkUtils::GetBroadcastIP(Utils::PlatformStringToStdString(host->ServerAddress));
+ if (broadcastIP != "") {
+ addresses.push_back(broadcastIP);
+ } else {
+ Utils::Log("Could not determine subnet mask from IP address.\n");
+ }
+ } catch (...) {
+ Utils::Log("Failed to calculate broadcast IP for WoL\n");
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (host->LastHostname) | |
| { | |
| auto hostnameSplit = Split_IP_Address(Utils::PlatformStringToStdString(host->LastHostname), ':'); | |
| auto hostnameSplit = NetworkUtils::SplitIPAddress(Utils::PlatformStringToStdString(host->LastHostname), ':'); | |
| std::string hostIp = hostnameSplit.first; | |
| addresses.push_back(hostIp); | |
| } | |
| if (inet_addr(Utils::PlatformStringToStdString(host->ServerAddress).c_str()) != -1) | |
| { | |
| std::string broadcastIP = Get_Broadcast_IP(Utils::PlatformStringToStdString(host->ServerAddress)); | |
| if (broadcastIP != "") | |
| std::string broadcastIP = NetworkUtils::GetBroadcastIP(Utils::PlatformStringToStdString(host->ServerAddress)); | |
| if (broadcastIP != "") | |
| { | |
| addresses.push_back(broadcastIP); | |
| } else { | |
| Utils::Log("Could not determine subnet mask from IP address.\n"); | |
| } | |
| if (host->LastHostname) | |
| { | |
| try { | |
| auto hostnameSplit = NetworkUtils::SplitIPAddress(Utils::PlatformStringToStdString(host->LastHostname), ':'); | |
| std::string hostIp = hostnameSplit.first; | |
| addresses.push_back(hostIp); | |
| } catch (...) { | |
| Utils::Log("Failed to parse LastHostname for WoL\n"); | |
| } | |
| } | |
| if (inet_addr(Utils::PlatformStringToStdString(host->ServerAddress).c_str()) != -1) | |
| { | |
| try { | |
| std::string broadcastIP = NetworkUtils::GetBroadcastIP(Utils::PlatformStringToStdString(host->ServerAddress)); | |
| if (broadcastIP != "") { | |
| addresses.push_back(broadcastIP); | |
| } else { | |
| Utils::Log("Could not determine subnet mask from IP address.\n"); | |
| } | |
| } catch (...) { | |
| Utils::Log("Failed to calculate broadcast IP for WoL\n"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @State/ApplicationState.cpp around lines 348 - 363, Wrap the calls to
NetworkUtils::SplitIPAddress and NetworkUtils::GetBroadcastIP in try-catch
blocks to prevent exceptions from propagating when LastHostname or ServerAddress
are malformed; catch std::invalid_argument and std::out_of_range (or
std::exception as a fallback), log an informative message via Utils::Log
(including the offending LastHostname/ServerAddress) and skip pushing results
into the addresses vector so the WoL flow continues safely. Ensure the try-catch
surrounds the SplitIPAddress call where host->LastHostname is used and the
GetBroadcastIP call where host->ServerAddress is used, and do not rethrow the
exceptions.
| static mdns_string_t ipv4_address_to_string(char *buffer, size_t capacity, const struct sockaddr_in *addr, size_t addrlen) { | ||
| char host[NI_MAXHOST] = { 0 }; | ||
| char service[NI_MAXSERV] = { 0 }; | ||
| int ret = getnameinfo((const struct sockaddr*)addr, (socklen_t)addrlen, host, NI_MAXHOST, | ||
| service, NI_MAXSERV, NI_NUMERICSERV | NI_NUMERICHOST); | ||
| int ret = getnameinfo( | ||
| (const struct sockaddr *)addr, (socklen_t)addrlen, | ||
| host, NI_MAXHOST, | ||
| service, NI_MAXSERV, | ||
| NI_NUMERICSERV | NI_NUMERICHOST); |
There was a problem hiding this comment.
Variable shadowing: local service shadows global service.
The local variable service (line 29) shadows the file-scope service constant (line 14). While this works correctly in the current code, it could cause confusion during maintenance.
Suggested fix: Rename local variable
static mdns_string_t ipv4_address_to_string(char *buffer, size_t capacity, const struct sockaddr_in *addr, size_t addrlen) {
char host[NI_MAXHOST] = { 0 };
- char service[NI_MAXSERV] = { 0 };
+ char svc[NI_MAXSERV] = { 0 };
int ret = getnameinfo(
(const struct sockaddr *)addr, (socklen_t)addrlen,
host, NI_MAXHOST,
- service, NI_MAXSERV,
+ svc, NI_MAXSERV,
NI_NUMERICSERV | NI_NUMERICHOST);
int len = 0;
if (ret == 0) {
if (addr->sin_port != 0) {
- len = snprintf(buffer, capacity, "%s:%s", host, service);
+ len = snprintf(buffer, capacity, "%s:%s", host, svc);
} else {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @State/MDNSHandler.cpp around lines 27 - 34, In ipv4_address_to_string, the
local variable named "service" shadows the file-scope "service" constant; rename
the local to something distinct (e.g., service_buf or service_local) and update
all uses within ipv4_address_to_string (including the getnameinfo call and any
subsequent references) to avoid the shadowing while preserving the buffer size
NI_MAXSERV.
| } else { | ||
| WSACleanup(); | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
Misplaced WSACleanup() call corrupts Winsock reference count.
GetBroadcastIP calls WSACleanup() on the error path (line 155), but this function doesn't call WSAStartup(). Since Winsock uses reference counting, this will decrement the count incorrectly, potentially breaking subsequent socket operations in other parts of the application.
Suggested fix: Remove the errant WSACleanup call
} else if ((ipAddress_int & 0xC0000000) == 0xC0000000) { // Class C
subnetMask = 4294967040; // 255.255.255.0
} else {
- WSACleanup();
return "";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| WSACleanup(); | |
| return ""; | |
| } | |
| } else if ((ipAddress_int & 0xC0000000) == 0xC0000000) { // Class C | |
| subnetMask = 4294967040; // 255.255.255.0 | |
| } else { | |
| return ""; | |
| } |
🤖 Prompt for AI Agents
In @Utils/NetworkUtils.cpp around lines 154 - 157, The GetBroadcastIP function
incorrectly calls WSACleanup() on its error path, which will decrement Winsock's
reference count even though GetBroadcastIP does not call WSAStartup(); remove
the errant WSACleanup() invocation from GetBroadcastIP (and any similar
early-return error branches) so Winsock cleanup is only performed by the code
that called WSAStartup() or a dedicated shutdown routine; if necessary, ensure
any WSAStartup() caller is responsible for a matching WSACleanup() instead of
doing cleanup inside GetBroadcastIP.
|
Using more this week, still seeing intermittent issues with host connections dropping. Going to continue to work on this. |
mpotrykus
changed the title
|
Opening back up. This was not the culprit for the network dropping issue, but it does add features and improvements. |
1 participant
Refactored the MDNSHandler and general network-handling.
Most notable improvements are:
Tested both IPv4 & IPv6:
Summary by CodeRabbit
New Features
Bug Fixes / Reliability