bugfix(network): Move transport socket bind into separate method by slurmlord · Pull Request #1478 · TheSuperHackers/GeneralsGameCode

slurmlord · 2025-08-19T19:07:37Z

This change removes the call to Transport::init from LANAPI::init, which avoids an unnecessary socket bind to IP 0x0 (INADDR_ANY). Correctness is maintained by Transport::init still being called from LANAPI::SetLocalIP.

Background

When launching multiple instances of the game in Wine, an assertion will trigger for all instances except the first one when navigating to the Network menu:

ASSERTION FAILURE: Could not bind to 0x00000000:8086

This is caused by LANAPI::init calling Transport::init, which will bind to IP address 0x0, the same as INADDR_ANY; all interfaces. This is due to LANAPI not having set the local IP address (m_localIP) that Transport should use, and instead using the default value of 0.
When LANAPI::SetLocalIP is later called, the local IP is assigned and Transport::init called again, which will bind to the same port as previous but on one specific IP. On Wine/Linux this results in the assertion above, as the port is still considered in use/unavailable for a longer period than on Windows.
By avoiding the init/bind from LANAPI::init, binding will only be done from LANAPI::SetLocalIP, where the proper address is used.

Validated by running a multi instance network game.

Caball009 · 2025-08-19T19:18:55Z

I have never encountered this issue and I use multiple instances frequently, usually 2, 3 or 4 instances.

slurmlord · 2025-08-19T19:34:29Z

I see - I'm running it via Wine (v10.12) which is probably the source of the difference in behavior.
The change actually fixes the problem in my case, but if the situation is too far off the supported path then perhaps it's just something I'll have to live with and abandon this PR?

Caball009 · 2025-08-19T19:42:28Z

I don't know if it has any drawbacks when running Windows natively. If this is a common issue for Wine then it seems worth fixing, but maybe behind a macro and with a comment that this is primarily for Wine.

xezon

ASSERTION FAILURE: Could not bind to 0x00000000:8086

Why is it binding to IP 0? How does this come to be? The clients should use 127.0.0.N, selectable in the Options Menu.

Generals/Code/GameEngine/Source/GameNetwork/udp.cpp

slurmlord · 2025-08-19T21:34:27Z

I don't know if it has any drawbacks when running Windows natively. If this is a common issue for Wine then it seems worth fixing, but maybe behind a macro and with a comment that this is primarily for Wine.

You mean a macro to at run-time detect if running on Wine? I can look into that.

slurmlord · 2025-08-19T21:37:00Z

ASSERTION FAILURE: Could not bind to 0x00000000:8086

Why is it binding to IP 0? How does this come to be? The clients should use 127.0.0.N, selectable in the Options Menu.

I can trace some more into this, perhaps there's something missing from the configuration somehow. But at the same time, games can be created, joined and played so it's at least mostly working.

This reverts commit bc8d52e.

slurmlord · 2025-08-20T19:54:01Z

Looking into the ordering of things, it became apparent that the binding to IP 0.0.0.0 happens because LANAPI::init calls m_transport->init(m_localIP, m_lobbyPort);
while m_localIP is still 0.
The function that sets m_localIP is LANAPI::SetLocalIP, which also re-initializes m_transport in the same way as in init.

I propose a fix in the latest iteration, where the initialization of m_transport is removed from init, and kept in SetLocalIP. The only calls to LANAPI::init I found are from LanLobbyMenuInit and NetworkDirectConnectInit. Both call LANAPI::SetLocalIP directly after LANAPI::init, so the effective behavior should be the same except the bind to 0.0.0.0.

There is some strangeness with LANAPI going on in the beginning of NetworkDirectConnectInit where it's created (if not already created) and deleted before being re-created again, but I didn't notice any difference in behavior here with the modified init.

Does this seem like a viable approach? If so, I'll update the title and description.

xezon · 2025-08-20T20:32:58Z

Needs some testing then to see if networking still works :)

slurmlord · 2025-08-22T06:46:50Z

So after discussing with @tomsons26, we landed on the underlying issue being that Transport binds its socket in the Transport::init method. Since LANAPI::init calls Transport::init before having an IP assigned yet, it results in binding to 0x0.

The solution is to split the socket binding out to a separate method in Transport so that LANAPI can init the Transport instance, but wait with the binding until LANAPI::SetLocalIP is called.

Validated to work for network game with both local multi-instance game and with 2 separate hosts over a physical network (all through Wine).

xezon · 2025-08-22T16:39:18Z

Generals/Code/GameEngine/Source/GameNetwork/Transport.cpp

+}

-	m_port = port;
+Bool Transport::bind(void)


You can potentially reduce the diff complexity by moving this function above clearBuffers

xezon · 2025-08-22T16:41:43Z

Generals/Code/GameEngine/Source/GameNetwork/NAT.cpp

 	DEBUG_LOG(("NAT::attachSlotList - using %d as the starting port number", m_startingPortNumber));
 	generatePortNumbers(slotList, localSlot);
 	m_transport->init(m_localIP, getSlotPort(localSlot));
+	m_transport->bind();


In LANAPI::SetLocalIP it calls bind only if init succeeded. Here it does not do that. Why?

Agreed - that pattern should be followed here, too.

xezon · 2025-08-22T16:42:53Z

Generals/Code/GameEngine/Source/GameNetwork/LANAPI.cpp

@@ -1258,6 +1257,10 @@ Bool LANAPI::SetLocalIP( UnsignedInt localIP )

 	m_transport->reset();
 	retval = m_transport->init(m_localIP, lobbyPort);


Bool success

xezon · 2025-08-22T16:44:23Z

Generals/Code/GameEngine/Source/GameNetwork/Transport.cpp

+	}
+
+	clearBuffers();
 }


Function does not return.

xezon · 2025-08-22T16:50:35Z

GeneralsMD/Code/GameEngine/Source/GameNetwork/LANAPI.cpp

@@ -102,7 +102,6 @@ void LANAPI::init( void )
 	m_gameStartSeconds = 0;
 	m_transport->reset();
 	m_transport->init(m_localIP, lobbyPort);


Is it even necessary to call init here then? Perhaps the simplest fix is to just remove

m_transport->init(m_localIP, lobbyPort); m_transport->allowBroadcasts(true);

?

If removing the above 2 lines is enough to fix the issue, then I would recommend to do that and then do a follow up refactor that splits init function into bind and clearBuffers like you have, but call the new functions in init right away.

So in practice go with just commit
ee4eb87 for this ? I think that change was the one that raised some concerns that the initialization of the LANAPI instance was expected to initialize all dependencies as well.

From the outside the behavior is the same; no bind is being done to 0x0. If this is acceptable I'll update the PR to just what was in ee4eb87.

ee4eb87 looks to be correct. Perhaps reinforce it by placing some asserts to verify that the transport is initialized before use.

xezon · 2025-08-23T12:09:23Z

Generals/Code/GameEngine/Source/GameNetwork/LANAPI.cpp


 Bool LANAPI::SetLocalIP( UnsignedInt localIP )
 {
+	DEBUG_ASSERTCRASH(m_currentGame == NULL && m_gameStartTime == 0, ("LANAPI::SetLocalIP called while in a game, should be called right after LANAPI::init"));


This is an odd assert because it will not protect from the error of not calling this function at all.

Better add a Transport::isInitialized function and assert on that in all relevant functions that use LANAPI::m_transport

slurmlord · 2025-09-01T15:50:39Z

Abandoning this PR as the main goal was to avoid an assert when running under Wine/Linux, but the networking behavior with regards to broadcasts is different enough that LAN play does not work as expected anyway.

The issue does not show up under Windows so leaving the code as is won't have any bad/worse consequences.

Set SO_REUSEADDR for network socket in multi-instance.

bc8d52e

xezon reviewed Aug 19, 2025

View reviewed changes

Generals/Code/GameEngine/Source/GameNetwork/udp.cpp Outdated Show resolved Hide resolved

slurmlord added 2 commits August 20, 2025 21:36

Revert "Set SO_REUSEADDR for network socket in multi-instance."

40abaa2

This reverts commit bc8d52e.

Remove reset and init of Transport before a local IP has been set.

ee4eb87

Move socket binding out of Transport::init to separate method.

d05c3c5

slurmlord changed the title ~~bugfix(network): Set SO_REUSEADDR for network socket in multi-instance~~ bugfix(network): Move transport socket bind into separate method Aug 22, 2025

Backport to Generals

69e16d6

xezon reviewed Aug 22, 2025

View reviewed changes

xezon added Bug Something is not working right, typically is user facing Minor Severity: Minor < Major < Critical < Blocker Network Anything related to network, servers Gen Relates to Generals ZH Relates to Zero Hour labels Aug 22, 2025

slurmlord added 2 commits August 23, 2025 10:40

Code review feedback

6005fbe

Simplify to not call Transport::init from LANAPI::init.

edabb55

xezon reviewed Aug 23, 2025

View reviewed changes

slurmlord closed this Sep 1, 2025

		@@ -1258,6 +1257,10 @@ Bool LANAPI::SetLocalIP( UnsignedInt localIP )

		m_transport->reset();
		retval = m_transport->init(m_localIP, lobbyPort);

Conversation

slurmlord commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Uh oh!

Caball009 commented Aug 19, 2025

Uh oh!

slurmlord commented Aug 19, 2025

Uh oh!

Caball009 commented Aug 19, 2025

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slurmlord commented Aug 19, 2025

Uh oh!

slurmlord commented Aug 19, 2025

Uh oh!

slurmlord commented Aug 20, 2025

Uh oh!

xezon commented Aug 20, 2025

Uh oh!

slurmlord commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slurmlord commented Sep 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

slurmlord commented Aug 19, 2025 •

edited

Loading

slurmlord commented Aug 22, 2025 •

edited

Loading