Skip to content

Part 6 - p2p: replace upnpclient with async-upnp-client for NAT port mapping#970

Open
ping-ke wants to merge 14 commits intoupgrade/py313-baselinefrom
upgrade/p2p-nat
Open

Part 6 - p2p: replace upnpclient with async-upnp-client for NAT port mapping#970
ping-ke wants to merge 14 commits intoupgrade/py313-baselinefrom
upgrade/p2p-nat

Conversation

@ping-ke
Copy link
Copy Markdown
Contributor

@ping-ke ping-ke commented Mar 15, 2026

Summary

upnpclient uses synchronous/blocking I/O and netifaces for interface discovery; neither is well-maintained on Python 3.13.

Rewrite UPnPService in nat.py to use async-upnp-client:

  • Use async_search() for UPnP IGD device discovery
  • Use AiohttpSessionRequester + UpnpFactory for async HTTP requests
  • Remove ThreadPoolExecutor blocking path and netifaces dependency
  • Expose discover() coroutine (previously add_nat_portmap()) that
    returns the mapped external IP string
  • Update p2p_server.py call site accordingly

Test plan

  • Node starts and attempts UPnP discovery without errors
  • NAT mapping is established when a compatible router is present
  • Graceful handling when no UPnP device is found (returns None)

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 ping-ke changed the title replace upnpclient with async-upnp-client for NAT port mapping p2p: replace upnpclient with async-upnp-client for NAT port mapping Mar 15, 2026
@ping-ke ping-ke changed the title p2p: replace upnpclient with async-upnp-client for NAT port mapping Part 6 - p2p: replace upnpclient with async-upnp-client for NAT port mapping Mar 15, 2026
@ping-ke ping-ke marked this pull request as draft March 17, 2026 06:43
ping-ke added 2 commits March 19, 2026 10:06
- 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
@ping-ke ping-ke marked this pull request as ready for review March 19, 2026 02:33
@ping-ke ping-ke requested review from qizhou, qzhodl and syntrust March 19, 2026 02:33
session = aiohttp.ClientSession()
try:
await self._discover(session)
finally:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may need to run the tests on a network with the router to verify that it works

@@ -1,48 +1,21 @@

import aiohttp
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be resolved after merge other changes. Test can be run on branch upgrade-py3-13.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ping-ke and others added 9 commits April 3, 2026 18:09
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
@ping-ke ping-ke changed the base branch from upgrade/py313-baseline to master April 4, 2026 16:06
@ping-ke ping-ke changed the base branch from master to upgrade/py313-baseline April 4, 2026 16:06
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