Part 6 - p2p: replace upnpclient with async-upnp-client for NAT port mapping#970
Open
ping-ke wants to merge 24 commits intoupgrade/py313-baselinefrom
Open
Part 6 - p2p: replace upnpclient with async-upnp-client for NAT port mapping#970ping-ke wants to merge 24 commits intoupgrade/py313-baselinefrom
ping-ke wants to merge 24 commits intoupgrade/py313-baselinefrom
Conversation
upnpclient uses synchronous I/O and netifaces for interface discovery, both incompatible or unmaintained under Python 3.13. Switch to async-upnp-client which provides asyncio-native UPnP/IGD support. Rewrite UPnPService.discover() to use async_search() for device discovery and AiohttpSessionRequester for requests, removing the blocking ThreadPoolExecutor path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ping-ke
changed the title
ping-ke
changed the title
- Fix AttributeError: replace undefined self.external_port/self.internal_port/self.protocol with self.port - Fix _discover timeout: wrap async_search with asyncio.wait_for instead of waiting after it completes - Fix _run: guard _add_port_mapping with self._service check to avoid AttributeError - Fix _delete_port_mapping: delete both TCP and UDP mappings to match AddPortMapping - Remove dead code: _refresh_task and _running fields that were never used
qzhodl
reviewed
Apr 1, 2026
qzhodl
reviewed
Apr 1, 2026
qzhodl
reviewed
Apr 1, 2026
qzhodl
reviewed
Apr 1, 2026
upnpclient uses synchronous I/O and netifaces for interface discovery, both incompatible or unmaintained under Python 3.13. Switch to async-upnp-client which provides asyncio-native UPnP/IGD support. Rewrite UPnPService.discover() to use async_search() for device discovery and AiohttpSessionRequester for requests, removing the blocking ThreadPoolExecutor path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix AttributeError: replace undefined self.external_port/self.internal_port/self.protocol with self.port - Fix _discover timeout: wrap async_search with asyncio.wait_for instead of waiting after it completes - Fix _run: guard _add_port_mapping with self._service check to avoid AttributeError - Fix _delete_port_mapping: delete both TCP and UDP mappings to match AddPortMapping - Remove dead code: _refresh_task and _running fields that were never used
…rkchain into upgrade/p2p-nat
- extract shared socket/aiohttp mocks into fixtures - extract fake_wait helper to reduce duplication - add tests for edge cases: exception handling, missing service, session cleanup - fix coroutine never awaited warning in test_run_exits_on_cancel
qzhodl
reviewed
Apr 7, 2026
qzhodl
reviewed
Apr 7, 2026
…andling - Override _cleanup() so stop() is called on service shutdown, preventing port mapping and aiohttp session leaks when the node exits - Close any existing session before creating a new one in discover() to avoid orphaned sessions on repeated calls - Handle SOAP error 718 (ConflictInMappingEntry) as success, matching the previous best-effort behavior from master - Roll back partial port mappings if AddPortMapping fails mid-loop - Import UpnpActionResponseError for typed SOAP error handling
Add _discover_lock so that concurrent callers cannot race on _session and _service state. The lock wraps the entire discover() body, ensuring each call fully completes (including session cleanup in the finally block) before the next begins.
… fix bool type, deduplicate - Guard on_response with early return if _service already found (SSDP sends one response per device sub-type, so the callback fires ~9x) - Extract location from CaseInsensitiveDict headers (newer async_upnp_client passes headers dict, not a response object) - Recursively traverse embedded_devices to find WANIPConnection service (root device only exposes Layer3Forwarding; WANIPConnection lives in the InternetGatewayDevice → WANDevice → WANConnectionDevice subtree) - Pass NewEnabled=True (bool) instead of 1; async_upnp_client validates the UPnP boolean type strictly
qzhodl
reviewed
Apr 14, 2026
qzhodl
reviewed
Apr 14, 2026
Use asyncio.Event + wait_for so _discover returns as soon as the first WANIPConn service is found instead of waiting the full 30-second timeout. Add post-await re-check to guard against concurrent on_response tasks that all pass the initial self._service guard before any of them completes.
Connect the UDP socket to the UPnP router's IP (parsed from the SSDP
location URL and stored in _router_host) instead of 8.8.8.8, so the OS
routing table selects the interface that actually reaches the router.
Matches the behaviour of the old netifaces-based
find_internal_ip_on_device_network() without the extra dependency.
Tests updated accordingly:
- test_get_internal_ip sets _router_host and asserts connect target
- Add test_get_internal_ip_no_router_host: returns None before discover()
- Add test_add_port_mapping_no_internal_ip: raises RuntimeError when
_router_host is None
- Fix fake_search to pass dict response so response.get('location') works
- Set fake_device.embedded_devices={} to prevent MagicMock recursion
- Move MOCK_ROUTER_HOST to top constants with clarifying comments
- Remove trivial guard tests (no_service, already_none, exits_on_cancel)
qzhodl
reviewed
Apr 15, 2026
qzhodl
approved these changes
Apr 17, 2026
blockchaindevsh
approved these changes
Apr 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
upnpclientuses synchronous/blocking I/O andnetifacesfor interface discovery; neither is well-maintained on Python 3.13.Rewrite
UPnPServiceinnat.pyto useasync-upnp-client:async_search()for UPnP IGD device discoveryAiohttpSessionRequester+UpnpFactoryfor async HTTP requestsThreadPoolExecutorblocking path andnetifacesdependencydiscover()coroutine (previouslyadd_nat_portmap()) thatreturns the mapped external IP string
p2p_server.pycall site accordinglyTest plan
None)