Skip to content

Fix event handler leaks of the live map page - #937

Open
sven-n wants to merge 1 commit into
masterfrom
claude/issue-919-xhslwj
Open

sven-n wants to merge 1 commit into
masterfrom
claude/issue-919-xhslwj

Conversation

@sven-n

@sven-n sven-n commented Sep 4, 2026

Copy link
Copy Markdown
Member

Fixes #919.

Every visit of the admin panel live map page (/gameServer/{id}/map/{mapId}) — and every terrain image request — permanently leaked one GameMapInfoAdapter per hosted map, because ObservableGameServerAdapter never disposed them. The leaked adapters stayed subscribed to the ObjectAdded / ObjectRemoved events of the live GameMaps, so they were rooted forever and additionally slowed down the game loop: those events are awaited sequentially on every spawn, drop, despawn and map transition.

Changes

The leak itself

  • ObservableGameServerAdapter.Dispose(bool) now unsubscribes and disposes its IGameMapInfos and clears the list. That's the actual fix — MapPage and GameServer already disposed the adapter, but the disposal didn't reach the map adapters.
  • OnGameMapCreated additionally disposes a map adapter which was created concurrently to the disposal, instead of adding it to the already cleared list.

Avoiding the expensive adapter creation altogether

  • InitializeAsync got a withMapInfos parameter (default true, so the Dapr singleton registration is unaffected). Callers which only need to register a map observer can skip creating and observing an adapter for every map on the server.
  • MapPage only needed the map infos to show the map name in the breadcrumb. It now initializes the adapter without them and resolves the name directly at the IGameServerContext.
  • TerrainController doesn't create an ObservableGameServerAdapter at all anymore. It looks the GameMap up at the context and renders the terrain through a new GetTerrainStream(this GameMap) overload. This resolves the existing // TODO: Do this without creating an ObservableGameServerAdapter, because that's a very expensive operation. The PNG cache is shared between both overloads, so nothing is rendered twice.

Component defects found in the same code

  • MapPlayerList subscribed to IMapController.PlayersChanged in OnParametersSet — which runs on every re-render of the parent Map component — and never unsubscribed. Handlers piled up, so after n parent renders a single player entering scope triggered n re-renders of the whole player table. It now subscribes only when the controller instance actually changed, keeps the handler in a field and unsubscribes in Dispose.
  • Map component: removed the synchronous Dispose. Task.Run(this.DisposeAsync).Wait() bound to Task.Run(Func<TResult>) and produced a Task<ValueTask>, so the inner ValueTask was never awaited — and it would have blocked the renderer thread. Blazor prefers IAsyncDisposable when a component implements both, so the remaining DisposeAsync is the path that was already taken.
  • MapController.DisposeAsync now awaits ObserverToWorldViewAdapter.DisposeAsync() instead of calling the synchronous no-op Dispose(), so its DisposeAsyncCore (clearing _observingObjects and asserting that ObservingBuckets is empty) actually runs.

Testing

  • dotnet build src/MUnique.OpenMU.sln -p:ci=true — succeeds, no new warnings in the touched files.
  • dotnet test tests/MUnique.OpenMU.Web.Tests — 88 passed, 0 failed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XyXbCxbtDecRjhoarSC8qP


Generated by Claude Code

Every visit of the admin panel live map page - and every terrain image
request - permanently leaked one GameMapInfoAdapter per hosted map,
because ObservableGameServerAdapter never disposed them. As the leaked
adapters stayed subscribed to the ObjectAdded/ObjectRemoved events of the
live game maps, they were rooted forever and additionally slowed down the
game loop, since these events are awaited on every spawn, drop, despawn
and map transition.

* ObservableGameServerAdapter now disposes (and unsubscribes) its map
  infos when it's disposed itself. A map adapter which was created
  concurrently to the disposal is disposed as well, instead of being
  added to the cleared list.
* InitializeAsync got a withMapInfos parameter, so callers which only
  need to register a map observer can skip the expensive creation of the
  map infos for all maps.
* MapPage uses that, and resolves the map name directly at the game
  server context.
* TerrainController doesn't create an ObservableGameServerAdapter
  anymore. It resolves the GameMap at the context and renders its terrain
  through a new GetTerrainStream overload, which resolves the TODO about
  the expensive adapter creation.
* MapPlayerList subscribed to IMapController.PlayersChanged on every
  OnParametersSet - which happens on each re-render of the parent
  component - and never unsubscribed. It now subscribes only when the
  controller instance actually changed and unsubscribes when it's
  disposed.
* Map component: removed the synchronous Dispose, which bound to
  Task.Run(Func<TResult>) and therefore never awaited the returned
  ValueTask of DisposeAsync.
* MapController now awaits ObserverToWorldViewAdapter.DisposeAsync
  instead of calling the no-op synchronous Dispose, so its
  DisposeAsyncCore actually runs.

Fixes #919

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XyXbCxbtDecRjhoarSC8qP
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.

Live map (admin panel): ObservableGameServerAdapter leaks a GameMapInfoAdapter per map on every page visit

2 participants