Part 6 - p2p: replace upnpclient with async-upnp-client for NAT port mapping#970
Part 6 - p2p: replace upnpclient with async-upnp-client for NAT port mapping#970ping-ke wants to merge 17 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
| @@ -1,48 +1,21 @@ | |||
|
|
|||
| import aiohttp | |||
There was a problem hiding this comment.
This PR introduces a new runtime dependency on async-upnp-client (and directly imports aiohttp in nat.py), but the project dependencies still only list upnpclient and netifaces.
Right now requirements.txt / setup.py are not updated to install the new dependency set, so a clean environment will fail at import time before this code can run.
It looks like we should add async-upnp-client (and ensure aiohttp is available through project dependencies), while removing the old UPnP-specific dependencies that are no longer used.
There was a problem hiding this comment.
This will be resolved after merge other changes. Test can be run on branch upgrade-py3-13.
There was a problem hiding this comment.
I don’t think this is a good practice, as additional bugs may be introduced during the merge process.
Also, have you run the full test suite again on upgrade-py3-13?
There was a problem hiding this comment.
All changes will be made on the upgrade-py3-13 branch. After all tests pass, they will be cherry-picked to the target branch. Once the merge is completed, the full test suite will be executed again. I will also perform a comparison between the upgrade/base-line branch and the upgrade-py3-13 branch to ensure consistency and correctness.
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
| # Public API | ||
| # ----------------------------- | ||
|
|
||
| async def discover(self) -> Optional[str]: |
There was a problem hiding this comment.
I think the error-handling behavior here is still riskier than the previous implementation.
Before this refactor, NAT setup was effectively best-effort: add_nat_portmap() logged common UPnP failures and returned None, so the server could still start even if port mapping failed. In the new flow, BaseServer._run() directly awaits self.upnp_service.discover(), and discover() only handles the “no WANIP service found” case gracefully.
If _discover() raises, or if _add_port_mapping() fails after a service has already been found, that exception now bubbles out of discover() and can abort server startup. In that failure path, the session also does not appear to be closed before the exception escapes.
It would be safer to preserve the previous best-effort behavior here: log the NAT setup failure, clean up the session, and return None so startup can continue without UPnP.
| @@ -0,0 +1,483 @@ | |||
| import asyncio | |||
There was a problem hiding this comment.
The added unit tests are useful, but I do not think they are enough on their own for this kind of networking change.
This PR replaces the UPnP implementation, changes how the internal IP is selected, and changes the mapping/session lifecycle. Those paths can pass with mocks while still failing against real routers or real local-network configurations.
Could we update the test results with at least one real-environment/manual validation run against an actual UPnP-capable router? That would make it much easier to assess whether the new implementation behaves correctly outside the mocked unit-test setup.
2 participants
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)