mirror of
https://github.com/hrydgard/ppsspp.git
synced 2026-10-30 19:24:23 +01:00
UPnP: fix the exit hang and the CPU spin, and only run the thread when enabled
The two long-standing bug reports had a shared root: the service loop could end up in a state it never left. - Exit hang: UPNP_CMD_EXIT was queued alongside port requests and only acted on when it reached the front. A request that couldn't complete was never popped, so exit sat behind it forever and join() blocked indefinitely. Exit is a flag now, checked before anything else. - CPU spin: wait_for() with a predicate returns immediately when the predicate already holds, so a stuck queue head meant a tight loop. sceNetInet's bind() queues UPnP_Add regardless of the setting, so this hit whenever UPnP was off and a game used sockets. The loop always blocks now, and requests are dropped while UPnP is off. - Failed discovery was retried every 5s forever, each time a full 2s SSDP round plus an error toast. Now backs off 5s -> 300s and reports once. Other things found while in here: - Every failed Initialize() leaked a UPNPUrls + IGDdatas, so ~every 5 seconds for anyone with UPnP on and no router. The manual miniwget/parserootdesc/ GetUPNPUrls block was also redundant - UPNP_GetValidIGD does all of it and memsets over the result, leaking the URLs and costing an extra HTTP round trip per attempt. - UPNP_GetValidIGD's status was never checked, so we could go DONE with no usable IGD and hand a NULL controlURL to UPNP_GetConnectionTypeInfo. - miniupnpc's strncpy into the port-mapping-entry buffers doesn't guarantee a terminator; an 80-char description ran std::string off the end of desc[80]. - Add() marked another app's port "taken" only after our own add succeeded, so a failed add left their mapping deleted and never restored. - Clear() walked the router's entire table at exit, one HTTP round trip per index. It now deletes only what we know we mapped, and the exit cleanup has a time budget so an unreachable router can't stall shutdown. - The in-flight request stayed in the queue during the router call, so a same-port request arriving concurrently could erase it and be dropped unexecuted. - The queue is bounded, and last-write-wins per port collapses the churn from games that rebind in a loop. - The mapping description is built when the request is queued rather than read off g_paramSFO from the UPnP thread later. The thread now only exists while the setting is on - turning it off makes it remove its mappings and exit, turning it on starts one. That means __UPnPInit() has to run after the config is actually loaded; g_Config.Init() only builds a lookup table. QueueRequest() reconciles too, so a per-game config or a libretro core option enabling UPnP works without a notify at every call site. The settings checkbox is disabled in-game, since sceNet latches related settings at boot and a game that already mapped its ports wouldn't cope with them disappearing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DCPmm7FoQUoqrbMdhfqhQ2
This commit is contained in:
1 parent
f89a2d4199
commit
2cb0be1bc4
5 files changed
+590
-427
No files matched your search
+5
-5
@@ -515,9 +515,6 @@ void NativeInit(int argc, const char *argv[], const CommandLineOptions &cmdLineO
|
||||
|
||||
IncrementDebugCounter(DebugCounter::APP_BOOT);
|
||||
|
||||
// Probably an excessive timeout. it only causes delays on shutdown, though.
|
||||
__UPnPInit(2000);
|
||||
|
||||
ShaderTranslationInit();
|
||||
|
||||
g_threadManager.Init(cpu_info.num_cores, cpu_info.logical_cpu_count);
|
||||
@@ -704,6 +701,11 @@ void NativeInit(int argc, const char *argv[], const CommandLineOptions &cmdLineO
|
||||
g_Config.LoadAppendedConfig();
|
||||
}
|
||||
|
||||
// Has to be after the config is loaded: it only starts a service thread if UPnP is enabled,
|
||||
// and g_Config.Init() above doesn't read the ini, it just builds a lookup table.
|
||||
// Probably an excessive timeout. It only causes delays on shutdown, though.
|
||||
__UPnPInit(2000);
|
||||
|
||||
// This parameter should be a boot filename. Only accept it if we
|
||||
// don't already have one.
|
||||
if (!cmdLineOptions.bootFilenames.empty()) {
|
||||
@@ -1851,8 +1853,6 @@ void NativeShutdown() {
|
||||
|
||||
__UPnPShutdown();
|
||||
|
||||
g_PortManager.Shutdown();
|
||||
|
||||
net::Shutdown();
|
||||
|
||||
g_Discord.Shutdown();
|
||||
|
||||
Reference in new issue
Block a user