test: close the listeners before terminating the event loop #35583

vasild commented at 4:47 PM on June 22, 2026: contributor

Whenever a test creates a new P2PInterface object a new listener is created inside NetworkThread.create_listen_server() by calling cls.network_event_loop.create_server().

These listeners are never closed which might result in:

2026-06-10T22:13:35.3934880Z Task was destroyed but it is pending!
2026-06-10T22:13:35.3936020Z task: <Task pending name='Task-54' coro=<BaseSelectorEventLoop._accept_connection2() done, defined at /opt/homebrew/Cellar/python@3.14/3.14.5/Frameworks/Python.framework/Versions/3.14/lib/python3.14/asyncio/selector_events.py:217> wait_for=<Future finished result=None>>

when the event loop is closed.

Fix that by closing the listeners.

Fixes: #35508

DrahtBot added the label Tests on Jun 22, 2026

DrahtBot commented at 4:47 PM on June 22, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35583.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	andrewtoth, sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

vasild commented at 4:54 PM on June 22, 2026: contributor

I can't reproduce the problem from #35508 locally, so I can't confirm 100% that this fixes it.

The reasoning is this - the error is coming from a Python asyncio listening server. We do not use those for the SOCKS5 proxy, but use them for the P2PInterface objects:

bitcoind --> SOCKS5 proxy --> P2PInterface

we collect the listeners in NetworkThread.listeners but only to check for duplicate listening addr:port and never close them. So, extend NetworkThread.close() to close all of them. This seems like a good hygiene anyway.

andrewtoth commented at 8:38 PM on June 22, 2026: contributor

I managed to get a diff that reproduces this locally by monkey-patching _accept_connection2:

diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 64dcbfd7ec..16a0805c4b 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -7,6 +7,7 @@
 import configparser
 from enum import Enum
 import argparse
+import asyncio
 from datetime import datetime, timezone
 from importlib.util import find_spec
 import logging
@@ -18,6 +19,7 @@ import random
 import re
 import shutil
 import subprocess
+import socket
 import sys
 import tempfile
 import time
@@ -281,6 +283,21 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
             pdb.set_trace()
 
         self.log.debug('Closing down network thread')
+
+        loop = self.network_thread.network_event_loop
+        make_transport = type(loop)._make_socket_transport
+
+        async def slow_accept(self, protocol_factory, conn, extra, sslcontext=None, server=None, *a, **k):
+            transport = make_transport(self, conn, asyncio.Protocol(), extra=extra, server=server)
+            try:
+                await asyncio.sleep(2)
+            finally:
+                transport.close()
+        type(loop)._accept_connection2 = slow_accept
+
+        addr, port = next(iter(NetworkThread.listeners))
+        self._repro_sock = socket.create_connection((addr, port))
+        time.sleep(0.3)
         self.network_thread.close(timeout=self.options.timeout_factor * 10)
         if self.success == TestStatus.FAILED:
             self.log.info("Not stopping nodes as test failed. The dangling processes will be cleaned up later.")

Confirmed this triggers the error every run on master, and does not trigger on this PR. Note: this fix only works on python > 3.12.1, so will still break on previous releases. (but wait, do we run the private broadcast tests on that CI job?)

DrahtBot added the label CI failed on Jun 22, 2026

vasild commented at 7:11 AM on June 24, 2026: contributor

this fix only works on python > 3.12.1

Why?

so will still break on previous releases. (but wait, do we run the private broadcast tests on that CI job?)

test_runner.py is ran, so "yes". Note that the change in this PR is not private broadcast specific.

vasild marked this as a draft on Jun 24, 2026

vasild commented at 8:25 AM on June 24, 2026: contributor

Converted to draft because this needs some more fiddling with.

vasild force-pushed on Jun 24, 2026

andrewtoth commented at 2:30 PM on June 24, 2026: contributor

this fix only works on python > 3.12.1

Why?

https://github.com/python/cpython/blob/3.12/Lib/asyncio/base_events.py#L390-L395

Sorry, should be >= 3.12.1.

vasild force-pushed on Jun 29, 2026

vasild force-pushed on Jun 30, 2026

DrahtBot removed the label CI failed on Jun 30, 2026

fanquake commented at 8:56 AM on August 17, 2026: member

What's the status here; is it still a draft? Note that the bug this is fixing is on the 32.x milestone.

test: close the listeners before terminating the event loop

Whenever a test creates a new `P2PInterface` object a new listener is
created inside `NetworkThread.create_listen_server()` by calling
`cls.network_event_loop.create_server()`.

These listeners are never closed which might result in:

```
2026-06-10T22:13:35.3934880Z Task was destroyed but it is pending!
2026-06-10T22:13:35.3936020Z task: <Task pending name='Task-54' coro=<BaseSelectorEventLoop._accept_connection2() done, defined at /opt/homebrew/Cellar/python@3.14/3.14.5/Frameworks/Python.framework/Versions/3.14/lib/python3.14/asyncio/selector_events.py:217> wait_for=<Future finished result=None>>
```

when the event loop is closed.

Fix that by closing the listeners.

Fixes: https://github.com/bitcoin/bitcoin/issues/35508

29fba5ddbb

test: close the loop after the network thread has completed

https://docs.python.org/3.15/library/asyncio-eventloop.html#asyncio.loop.close
reads "The loop must not be running when this function is called". It
seems safer to call `close()` after the thread has exited.

e4d80e7001

vasild force-pushed on Aug 18, 2026

vasild commented at 4:22 AM on August 18, 2026: contributor

c4a1f81f4f813cc4ae648b2ebf9c6a34384c925a...e4d80e7001e9996a2836225f45a905dd16ffc777: rebase and drop the temporary debug-print commit.

What's the status here; is it still a draft? Note that the bug this is fixing is on the 32.x milestone.

It was draft because the commit I added to help analyze it. Dropped it now and opening for review.

vasild marked this as ready for review on Aug 18, 2026

in test/functional/test_framework/p2p.py:771 in e4d80e7001

 766 | +
 767 | +        async def close_listeners():
 768 | +            for listener in listeners:
 769 | +                listener.close()
 770 | +            for listener in listeners:
 771 | +                await listener.wait_closed()

vasild commented at 4:37 AM on August 18, 2026:

This can be simplified in Python >= 3.13 which introduced close_clients(). The peer_disconnect() loop can then be removed and a close_clients() call can be added after wait_closed().

in test/functional/test_framework/p2p.py:814 in e4d80e7001

 810 | @@ -795,6 +811,7 @@ def peer_protocol():
 811 |              response = cls.protos.get((addr, port))
 812 |              # remove protocol function from dict only when reconnection doesn't need to happen/already happened
 813 |              if not proto.reconnect:
 814 | +                cls.protos_accept_done.append(response)

vasild commented at 4:55 AM on August 18, 2026:

comment from @l0rinc + AI:

peer_protocol() can append None to protos_accept_done — a second inbound connection on a non-reconnect listener gets response = None (dict entry already consumed), and close() would then die with AttributeError: 'NoneType' object has no attribute 'peer_disconnect'. Narrow teardown race, one-line guard (if response is not None) fixes it.

That would be:

-                cls.protos_accept_done.append(response)
+                if response is not None:
+                    cls.protos_accept_done.append(response)

but is it really necessary? If response happens to be None, then create_server() would be upset about it? https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.create_server:

protocol_factory must be a callable returning a protocol implementation.

So, in our functional tests response is never None because we would have seen failures if it was?

andrewtoth commented at 5:15 PM on August 23, 2026:

It looks like create_server() just stores protocol_factory, but does not call it. It is called later in _accept_connection2() after create_server() returns. If it returns None there the exception is swallowed.

However, response can indeed not be None here, so this guard is not needed. cls.protos[(addr, port)] = proto is always set below, and won't be set to None before the callback which gets us here. This can only happen if a second connection reuses the same listener without going through listen() again, which none do today.

So, I would recommend keeping this as is.

sedited commented at 3:30 PM on August 24, 2026:

Why is this gated behind proto.reconnect? To me it reads like a no-op if we disconnect again. Might there be a scenario where we reconnect but still need to clean up the connection?

andrewtoth commented at 3:53 PM on August 24, 2026:

Related to #35583 (review).

It could be moved out of the gate. This would be for a v2->v1 downgrade scenario, which none of our test cases exercise without first closing all connections before NetworkThread.close is called.

andrewtoth approved

andrewtoth commented at 5:28 PM on August 23, 2026: contributor

ACK e4d80e7001e9996a2836225f45a905dd16ffc777

LGTM. The reproducer #35583 (comment) still works.

maflcko added this to the milestone 32.0 on Aug 24, 2026

sedited approved

sedited commented at 9:35 PM on August 24, 2026: contributor

ACK e4d80e7001e9996a2836225f45a905dd16ffc777