feat(network): Self-healing NAT state by githubawn · Pull Request #3010 · TheSuperHackers/GeneralsGameCode

githubawn · 2026-07-23T22:06:47Z

What this code did:

The ButtonFirewallRefresh button in OptionsMenu.cpp and FirewallNeedToRefresh in FirewallHelper.cpp / OptionPreferences.cpp:

Saved/read FirewallNeedToRefresh and LastFirewallIP boolean flags to/from Options.ini.
Required players to manually click a "Refresh NAT" button in the Options GUI when changing network interfaces or encountering P2P negotiation failures.
Contaminated GlobalData (TheWritableGlobalData->m_firewallBehavior) with disk-persisted firewall state across process restarts.

How the new code works:

Automated In-Engine RAM Lifecycle: NAT state classification (m_behavior) is managed 100% in memory within FirewallHelperClass.
Transparent Background Probing: Non-blocking STUN probing runs in the background during online lobby entry (WOLWelcomeMenu), caching the classified result in RAM for the remainder of the game session.
Self-Healing Re-Detection: If a peer connection times out (NAT.cpp) or the user selects a new IP address in Options (OptionPreferences.cpp), TheFirewallHelper->flagNeedToRefresh(TRUE) resets m_behavior = FIREWALL_TYPE_UNKNOWN in RAM to seamlessly trigger a fresh background probe.
Clean GUI Deprecation: Hides OptionsMenu.wnd:ButtonFirewallRefresh via winHide(TRUE) in C++ without breaking custom or legacy .wnd layout files.

Background & Reason for Removal:

Obsolete Manual Workaround: Manual NAT refresh buttons are a legacy 2003 workaround; modern network stacks handle NAT re-detection automatically in-engine.
Elimination of Disk I/O: Completely removes FirewallNeedToRefresh and LastFirewallIP from Options.ini, preventing stale or corrupted flags from persisting across game crashes or process restarts.
Architectural Decoupling: Fully encapsulates STUN probing state inside FirewallHelperClass in memory, removing direct mutations to TheWritableGlobalData->m_firewallBehavior.

greptile-apps · 2026-07-23T22:09:34Z

Greptile Summary

The PR moves NAT classification from persisted global preferences into a session-lived FirewallHelper.

Automatically starts and advances background firewall detection.
Restarts detection after an IP change or failed NAT negotiation.
Retains completed detection results for matchmaking and removes the manual refresh workflow.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains in the fixes associated with the previous review threads.

Important Files Changed

Filename	Overview
Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp	Centralizes the in-memory detection lifecycle, performs a complete socket/state reset on refresh, and preserves completed classifications.
Core/GameEngine/Source/GameNetwork/NAT.cpp	Retains the shared helper across connection setup and requests a fresh classification after negotiation failure.
Core/GameEngine/Source/Common/OptionPreferences.cpp	Invalidates the active NAT classification when the selected LAN or online address changes.
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLWelcomeMenu.cpp	Advances background detection without deleting its session-lived result.
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLWelcomeMenu.cpp	Mirrors the persistent background-detection lifecycle for Zero Hour.
Generals/Code/GameEngine/Source/GameNetwork/GameSpy.cpp	Continues firewall probing from the GameSpy update loop while retaining the resulting helper state.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Enter online lobby] --> B{FirewallHelper exists?}
  B -- No --> C[Create helper and start detection]
  B -- Yes --> D[Reuse session helper]
  C --> E[Advance probe during updates]
  D --> E
  E --> F[Cache NAT behavior in memory]
  F --> G[Game setup and quick match consume behavior]
  H[Selected IP changes] --> I[Reset probe state and sockets]
  J[NAT negotiation fails] --> I
  I --> E

_{Reviews (5): Last reviewed commit: "fix(network): Preserve FirewallHelper li..." | Re-trigger Greptile}

### What this code did: The `ButtonFirewallRefresh` button in `OptionsMenu.cpp` and `FirewallNeedToRefresh` in `FirewallHelper.cpp` / `OptionPreferences.cpp`: 1. Saved/read `FirewallNeedToRefresh` and `LastFirewallIP` boolean flags to/from `Options.ini`. 2. Required players to manually click a "Refresh NAT" button in the Options GUI when changing network interfaces or encountering P2P negotiation failures. 3. Contaminated `GlobalData` (`TheWritableGlobalData->m_firewallBehavior`) with disk-persisted firewall state across process restarts. ### How the new code works: 1. **Automated In-Engine RAM Lifecycle**: NAT state classification (`m_behavior`) is managed 100% in memory within `FirewallHelperClass`. 2. **Transparent Background Probing**: Non-blocking STUN probing runs in the background during online lobby entry (`WOLWelcomeMenu`), caching the classified result in RAM for the remainder of the game session. 3. **Self-Healing Re-Detection**: If a peer connection times out (`NAT.cpp`) or the user selects a new IP address in Options (`OptionPreferences.cpp`), `TheFirewallHelper->flagNeedToRefresh(TRUE)` resets `m_behavior = FIREWALL_TYPE_UNKNOWN` in RAM to seamlessly trigger a fresh background probe. 4. **Clean GUI Deprecation**: Hides `OptionsMenu.wnd:ButtonFirewallRefresh` via `winHide(TRUE)` in C++ without breaking custom or legacy `.wnd` layout files. ### Background & Reason for Removal: - **Obsolete Manual Workaround**: Manual NAT refresh buttons are a legacy 2003 workaround; modern network stacks handle NAT re-detection automatically in-engine. - **Elimination of Disk I/O**: Completely removes `FirewallNeedToRefresh` and `LastFirewallIP` from `Options.ini`, preventing stale or corrupted flags from persisting across game crashes or process restarts. - **Architectural Decoupling**: Fully encapsulates STUN probing state inside `FirewallHelperClass` in memory, removing direct mutations to `TheWritableGlobalData->m_firewallBehavior`.

…e-detection

…d matchmaking

Skyaero42 · 2026-07-25T06:39:38Z

 		FirewallHelperClass();
 		virtual ~FirewallHelperClass();
 		Bool detectFirewall();
+		void detectFirewallBehavior(/*Bool &canRecord*/);


Why is the argument commented out?

Skyaero42 · 2026-07-25T06:42:02Z

+	if ((*this)["IPAddress"].compareNoCase(IP) != 0)
+	{
+		(*this)["IPAddress"] = IP;
+		if (TheFirewallHelper != nullptr)


nit: Assignment of IP and resetting refresh flag of firewallhelper are two non-related commands. An empty line between the assignment and the if statement would have my preference for higher readability.

Skyaero42 · 2026-07-25T06:46:27Z

 void OptionPreferences::setOnlineIPAddress(AsciiString IP)
 {
-	(*this)["GameSpyIPAddress"] = IP;
+	if ((*this)["GameSpyIPAddress"].compareNoCase(IP) != 0)


This code is a duplication of setLanIPAddress(). IMO it is better to have a private function with this logic and the two public functions calling the private function, i.e.:

void OptionPreferences::setLANIPAddress(AsciiString IP) { setIpAddress(IP); } void OptionPreferences::setOnlineIPAddress(AsciiString IP) { setIpAddress(IP); } void setIpAddress(AsciiString IP) { if ((*this)["IPAddress"].compareNoCase(IP) != 0) { (*this)["IPAddress"] = IP; if (TheFirewallHelper != nullptr) TheFirewallHelper->flagNeedToRefresh(TRUE); } }

Skyaero42 · 2026-07-25T06:49:01Z

 void OptionPreferences::setLANIPAddress(AsciiString IP)
 {
-	(*this)["IPAddress"] = IP;
+	if ((*this)["IPAddress"].compareNoCase(IP) != 0)


Rather than putting all the logic within an if statement, you could tap out early

if ((*this)["IPAddress"].compareNoCase(IP) == 0) return; (*this) ....

This increases readability and maintainability.

Skyaero42 · 2026-07-25T06:51:04Z


 void OptionPreferences::setOnlineIPAddress(UnsignedInt IP)
 {
 	AsciiString tmp;


As this code is touched, maybe also change the naming of the parameter tmp as it is a poor name that doesn't reflect its purpose.

Skyaero42 · 2026-07-25T06:51:41Z

+		delete TheFirewallHelper;
+		TheFirewallHelper = nullptr;
+	}
+	FirewallHelperClass *helper = NEW FirewallHelperClass();


nit: add a whiteline after the closing bracket of an if-statement for higher readability

Skyaero42 · 2026-07-25T06:56:04Z

 FirewallHelperClass * createFirewallHelper()
 {
-	return NEW FirewallHelperClass();
+	if (TheFirewallHelper)


I am confused by the logic here. createFirewallHelper creates a new FirewallHelperClass and assigns it to a local variable, then returns that variable.

But first it deletes the instance (if any) of the global TheFirewallHelper parameter.

This reads like this function is not adhering to single responsibility.

githubawn changed the title ~~feat(network): make NAT state self-healing~~ feat(network): Self-healing NAT state Jul 23, 2026

greptile-apps Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp

githubawn force-pushed the refactor/remove-refresh-nat branch from c5806f1 to 79e5c8e Compare July 23, 2026 22:14

githubawn force-pushed the refactor/remove-refresh-nat branch from 79e5c8e to 8113c09 Compare July 23, 2026 22:18

greptile-apps Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp

githubawn added 2 commits July 24, 2026 00:25

fix(network): Reset probe state and close spare sockets on firewall r…

0a4ada6

…e-detection

fix(network): Preserve FirewallHelper lifetime across online menus an…

fd146ae

…d matchmaking

Skyaero42 reviewed Jul 25, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(network): Self-healing NAT state#3010

feat(network): Self-healing NAT state#3010
githubawn wants to merge 3 commits into
TheSuperHackers:mainfrom
githubawn:refactor/remove-refresh-nat

githubawn commented Jul 23, 2026

Uh oh!

greptile-apps Bot commented Jul 23, 2026 •

edited

Loading

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

Uh oh!

Skyaero42 Jul 25, 2026

Uh oh!

Skyaero42 Jul 25, 2026

Uh oh!

Skyaero42 Jul 25, 2026

Uh oh!

Skyaero42 Jul 25, 2026

Uh oh!

Skyaero42 Jul 25, 2026

Uh oh!

Skyaero42 Jul 25, 2026

Uh oh!

Skyaero42 Jul 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

githubawn commented Jul 23, 2026

What this code did:

How the new code works:

Background & Reason for Removal:

Uh oh!

greptile-apps Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

Uh oh!

Skyaero42 Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

Skyaero42 Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

Skyaero42 Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

Skyaero42 Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

Skyaero42 Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

Skyaero42 Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

Skyaero42 Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

greptile-apps Bot commented Jul 23, 2026 •

edited

Loading