Skip to content

Make the texture send queue own its packet copies - #73

Open
martinecker wants to merge 1 commit into
sammyfreg:masterfrom
martinecker:fix/texture-command-queue-lifetime
Open

Make the texture send queue own its packet copies#73
martinecker wants to merge 1 commit into
sammyfreg:masterfrom
martinecker:fix/texture-command-queue-lifetime

Conversation

@martinecker

Copy link
Copy Markdown

Change TexturePendingServerAdd() to clone each CmdTexture before adding it to the send queue. This keeps objects in mTrackedTextures separate from packets being used by the communication thread.

Update TextureTrackingRem() to queue a Destroy copy and immediately remove and free the tracked Create object. Free temporary managed-texture Update objects after their queue copies are created. Make Communications_Outgoing_Textures() release every queued texture packet after it is sent.

Add Communications_ReleaseTexturePackets() to free pending and in-flight texture packets when Communications_Loop() disconnects or ClientInfo is destroyed. Stop accepting texture packets after mbDisconnectPending is set, clear packets from the old connection, and remove older pending packets when a Create or Destroy supersedes them.

Walk mTrackedTextures backward when TextureTrackingRem() can erase entries. Update ClientInfo::TextureTrackingClear() to preserve texture IDs supplied by legacy ImGui renderers and generate an ID only when the texture does not already have one.

These changes prevent texture packets from being changed or freed while they are being sent. They also prevent stale Updates from reaching a new server before their Creates and avoid leaking packets after an interrupted connection.

Change TexturePendingServerAdd() to clone each CmdTexture before adding it to the send queue. This keeps objects in mTrackedTextures separate from packets being used by the communication thread.

Update TextureTrackingRem() to queue a Destroy copy and immediately remove and free the tracked Create object. Free temporary managed-texture Update objects after their queue copies are created. Make Communications_Outgoing_Textures() release every queued texture packet after it is sent.

Add Communications_ReleaseTexturePackets() to free pending and in-flight texture packets when Communications_Loop() disconnects or ClientInfo is destroyed. Stop accepting texture packets after mbDisconnectPending is set, clear packets from the old connection, and remove older pending packets when a Create or Destroy supersedes them.

Walk mTrackedTextures backward when TextureTrackingRem() can erase entries. Update ClientInfo::TextureTrackingClear() to preserve texture IDs supplied by legacy ImGui renderers and generate an ID only when the texture does not already have one.

These changes prevent texture packets from being changed or freed while they are being sent. They also prevent stale Updates from reaching a new server before their Creates and avoid leaking packets after an interrupted connection.
@sammyfreg

Copy link
Copy Markdown
Owner

Could you explain the problem encountered leading to this change?

@martinecker

Copy link
Copy Markdown
Author

This came about from an infinite loop we hit in TexturePendingServerAdd(). The same CmdTexture was being queued twice. This led to its mpNext pointint back to itself and then on next list traversal the game was stuck in an infinite loop.

While attempting to fix that we found texture commands leaking, which then led to making ownership explicit. Now the tracking list keeps the texture data, and the send queue gets its own packet copies. These are freed after sending or during disconnect cleanup.

So the change started with the infinite loop and eventually grew to fix the allocation and cleanup problems we found along the way.

@sammyfreg

sammyfreg commented Sep 5, 2026 via email

Copy link
Copy Markdown
Owner

@martinecker

Copy link
Copy Markdown
Author

Okay, let me try to reconstruct what we saw. It's been a little since we've made these changes, so please excuse if my memory is foggy...

NetImgui keeps a texture’s Create command in mTrackedTextures and also uses that same object as a node in the send queue.
IIRC our flow was something like this to get into the infinite loop state:

  1. Texture A’s Create packet was tracked and queued.
  2. Another texture upload used the same texture ID.
  3. TextureTrackingAdd() called TextureTrackingRem() to remove the previous texture.
  4. TextureTrackingRem() changed the existing packet from Create to Destroy and queued that same pointer again.
    If the original packet was still pending, it was now being inserted into a list that already had it. This would make mpNext point back to itself causing a later traversal to loop forever.

Another scenario that we hit if I recall was during connection setup where ContextOverride() resends every tracked Create packet, which could also requeue a packet that was already pending.

@sammyfreg

Copy link
Copy Markdown
Owner

The dev branch has some recent code refactoring to better handle textures. This change might not be needed anymore and is probably not compatible with the latest dev branch,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants