-
Notifications
You must be signed in to change notification settings - Fork 231
feat: enhance BasicHost shutdown #1208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
1b27fe3
817f6d3
6f8fb31
914ce88
f06f3e1
5193734
cbebae1
2f0d637
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -279,6 +279,7 @@ def __init__( | |
| # Automatic identify coordination | ||
| self._identify_inflight: set[ID] = set() | ||
| self._identified_peers: set[ID] = set() | ||
| self._identify_scopes: dict[ID, trio.CancelScope] = {} | ||
| self._network.register_notifee(_IdentifyNotifee(self)) | ||
|
|
||
| def get_id(self) -> ID: | ||
|
|
@@ -978,6 +979,34 @@ async def disconnect(self, peer_id: ID) -> None: | |
| await self._network.close_peer(peer_id) | ||
|
|
||
| async def close(self) -> None: | ||
| """ | ||
| Close the host and its underlying network service. | ||
|
|
||
| Cleanup order is intentional: stop background services, remove UPnP port | ||
| mappings, cancel inflight identify tasks, then close the network. | ||
| """ | ||
| # Stop background services | ||
| if self.mDNS is not None: | ||
| self.mDNS.stop() | ||
|
|
||
| if self.bootstrap is not None: | ||
| self.bootstrap.stop() | ||
|
|
||
| # Cleanup UPnP mappings if active | ||
| if self.upnp and self.upnp.get_external_ip(): | ||
| try: | ||
| logger.debug("Removing UPnP port mappings due to host closure") | ||
| for addr in self.get_addrs(): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small inconsistency: this uses Functionally both work since |
||
| if port := addr.value_for_protocol("tcp"): | ||
| await self.upnp.remove_port_mapping(int(port), "TCP") | ||
| except Exception as e: | ||
| logger.warning(f"Error removing UPnP mappings during close: {e}") | ||
|
|
||
| # Cancel inflight identify tasks | ||
| for scope in list(self._identify_scopes.values()): | ||
| scope.cancel() | ||
|
|
||
| # Close network | ||
| await self._network.close() | ||
|
|
||
| def _schedule_identify(self, peer_id: ID, *, reason: str) -> None: | ||
|
|
@@ -993,14 +1022,28 @@ def _schedule_identify(self, peer_id: ID, *, reason: str) -> None: | |
| return | ||
| if not self._should_identify_peer(peer_id): | ||
| return | ||
|
|
||
| self._identify_inflight.add(peer_id) | ||
| trio.lowlevel.spawn_system_task(self._identify_task_entry, peer_id, reason) | ||
|
|
||
| async def _identify_task_entry(self, peer_id: ID, reason: str) -> None: | ||
| # Create a new cancel scope for this identify task | ||
| cancel_scope = trio.CancelScope() | ||
| self._identify_scopes[peer_id] = cancel_scope | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This blank line still has trailing whitespace — ruff fails CI with |
||
| trio.lowlevel.spawn_system_task( | ||
| self._identify_task_entry, peer_id, reason, cancel_scope | ||
| ) | ||
|
|
||
| async def _identify_task_entry( | ||
| self, peer_id: ID, reason: str, cancel_scope: trio.CancelScope | ||
| ) -> None: | ||
| try: | ||
| await self._identify_peer(peer_id, reason=reason) | ||
| with cancel_scope: | ||
| await self._identify_peer(peer_id, reason=reason) | ||
| finally: | ||
| self._identify_inflight.discard(peer_id) | ||
| # Remove scope from tracking if it matches (to avoid race conditions) | ||
| if self._identify_scopes.get(peer_id) is cancel_scope: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call on the |
||
| self._identify_scopes.pop(peer_id, None) | ||
|
|
||
| def _has_cached_protocols(self, peer_id: ID) -> bool: | ||
| """ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| BasicHost and test infrastructure now perform proper shutdown and resource cleanup, eliminating "Task was destroyed but it is pending!" warnings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not blocking, but worth thinking about: calling
close()twice could double-invokeself.mDNS.stop()→zeroconf.close(), which may raise. And there's a potential overlap with therun()context manager'sfinallyblock (L430-439) which already stops mDNS and removes UPnP mappings.A simple
_closedflag in__init__checked at the top of this method would make it idempotent and safe in either ordering. @acul71 raised the same concern. Up to you — just flagging the edge case.