Skip to content

Commit e7f914b

Browse files
committed
gh-130141: clean up asyncio._SelectorTransport in __del__
__del__ closed the socket but stopped there: the file descriptor stayed registered with the event loop and self._sock_fd kept pointing at a number the OS is now free to hand out to anything else. So the loop could go on polling a descriptor the transport no longer owned, and a transport resurrected during garbage collection could later call close() and unregister an unrelated descriptor from the selector, hanging whoever was waiting on it. Leave the transport in the state that _force_close() and _call_connection_lost() would instead: clear the buffer and drop the writer, mark the transport as closing and drop the reader, invalidate _sock_fd before closing the socket, then unlink the protocol, the loop and the server. All of it stays under "if self._sock is not None", so a finalizer running on a transport that has already lost its socket still does nothing. That matters for the server link especially: _call_connection_lost() calls server._detach() before it clears self._server, so a _detach() that raises leaves _server set on a transport whose socket is gone, and an unguarded finalizer would then detach a second time. The ResourceWarning is issued first, while repr() can still report the fd and the selector state. _call_connection_lost() also sets _sock_fd to -1 now, so the "_sock_fd is valid only while _sock is open" invariant holds on every path that closes the socket; as a result repr() of a closed transport reports fd=-1. This is the patch Guido van Rossum worked out in the issue thread, rebased, with the cleanup kept inside the socket guard and extended with the _buffer_size reset that current main needs, plus the unit tests the earlier attempt was missing.
1 parent 0546b5f commit e7f914b

3 files changed

Lines changed: 132 additions & 3 deletions

File tree

‎Lib/asyncio/selector_events.py‎

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -878,11 +878,39 @@ def close(self):
878878
self._call_soon(self._call_connection_lost, None)
879879

880880
def __del__(self, _warn=warnings.warn):
881+
# The transport can be resurrected after this runs, so leave it in the
882+
# state _force_close() and _call_connection_lost() would: the fd is
883+
# gone, and a stale self._sock_fd would later be used to unregister a
884+
# descriptor the OS has handed out to somebody else.
881885
if self._sock is not None:
882-
_warn(f"unclosed transport {self!r}", ResourceWarning, source=self)
886+
# Warn before cleaning up: the message embeds repr(self), which
887+
# reports the fd number.
888+
if self._protocol_connected:
889+
self._protocol_connected = False
890+
_warn(f"unclosed transport {self!r}", ResourceWarning,
891+
source=self)
892+
893+
if self._buffer:
894+
self._buffer.clear()
895+
self._buffer_size = 0
896+
self._loop._remove_writer(self._sock_fd)
897+
898+
if not self._closing:
899+
self._closing = True
900+
self._loop._remove_reader(self._sock_fd)
901+
902+
self._conn_lost += 1
903+
904+
self._sock_fd = -1
883905
self._sock.close()
884-
if self._server is not None:
885-
self._server._detach(self)
906+
self._sock = None
907+
self._protocol = None
908+
self._loop = None
909+
910+
server = self._server
911+
if server is not None:
912+
self._server = None
913+
server._detach(self)
886914

887915
def _fatal_error(self, exc, message='Fatal error on transport'):
888916
# Should be called from exception handler only.
@@ -914,8 +942,10 @@ def _force_close(self, exc):
914942
def _call_connection_lost(self, exc):
915943
try:
916944
if self._protocol_connected:
945+
self._protocol_connected = False
917946
self._protocol.connection_lost(exc)
918947
finally:
948+
self._sock_fd = -1
919949
self._sock.close()
920950
self._sock = None
921951
self._protocol = None

‎Lib/test/test_asyncio/test_selector_events.py‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -542,6 +542,75 @@ def test_force_close(self):
542542
self.assertFalse(self.loop.readers)
543543
self.assertEqual(1, self.loop.remove_reader_count[7])
544544

545+
def test_del(self):
546+
tr = self.create_transport()
547+
self.loop._add_reader(7, mock.sentinel)
548+
549+
with self.assertWarns(ResourceWarning):
550+
tr.__del__()
551+
552+
# The socket is closed, so fd 7 may be handed out to an unrelated
553+
# file at any moment: the loop must not be left polling it, and the
554+
# cached fd must no longer look valid.
555+
self.assertFalse(self.loop.readers)
556+
self.assertEqual(1, self.loop.remove_reader_count[7])
557+
self.assertEqual(-1, tr._sock_fd)
558+
self.sock.close.assert_called_with()
559+
self.assertIsNone(tr._sock)
560+
self.assertIsNone(tr._protocol)
561+
self.assertIsNone(tr._loop)
562+
self.assertTrue(tr.is_closing())
563+
564+
def test_del_write_buffer(self):
565+
tr = self.create_transport()
566+
tr._buffer.extend(b'data')
567+
tr._buffer_size = 4
568+
self.loop._add_reader(7, mock.sentinel)
569+
self.loop._add_writer(7, mock.sentinel)
570+
571+
with self.assertWarns(ResourceWarning):
572+
tr.__del__()
573+
574+
self.assertFalse(self.loop.readers)
575+
self.assertFalse(self.loop.writers)
576+
self.assertEqual(1, self.loop.remove_writer_count[7])
577+
self.assertEqual(tr._buffer, list_to_buffer())
578+
self.assertEqual(0, tr.get_write_buffer_size())
579+
580+
def test_del_warning_names_the_fd(self):
581+
# The warning has to be issued before the cleanup, otherwise its
582+
# repr(self) reports a transport that is already closed and the fd
583+
# number, the only actionable part of the message, is lost.
584+
tr = self.create_transport()
585+
586+
with self.assertWarns(ResourceWarning) as cm:
587+
tr.__del__()
588+
589+
self.assertIn('fd=7', str(cm.warning))
590+
591+
def test_del_then_close_leaves_reused_fd_alone(self):
592+
tr = self.create_transport()
593+
self.loop._add_reader(7, mock.sentinel)
594+
595+
with self.assertWarns(ResourceWarning):
596+
tr.__del__()
597+
598+
# Something else in the process now owns fd 7 and waits on it.
599+
self.loop._add_reader(7, mock.sentinel.other)
600+
self.loop._add_writer(7, mock.sentinel.other)
601+
self.loop.reset_counters()
602+
603+
# The transport got resurrected during garbage collection and is
604+
# closed properly by its new owner. It no longer owns fd 7, so it
605+
# must keep its hands off the loop.
606+
tr.close()
607+
tr.abort()
608+
609+
self.assertEqual(0, self.loop.remove_reader_count[7])
610+
self.assertEqual(0, self.loop.remove_writer_count[7])
611+
self.assertIs(mock.sentinel.other, self.loop.readers[7]._callback)
612+
self.assertIs(mock.sentinel.other, self.loop.writers[7]._callback)
613+
545614
@mock.patch('asyncio.log.logger.error')
546615
def test_fatal_error(self, m_exc):
547616
exc = OSError()
@@ -599,6 +668,31 @@ def test__add_reader(self):
599668
self.assertFalse(self.loop.readers)
600669

601670

671+
class SelectorTransportDelTests(test_utils.TestCase):
672+
"""__del__ against a real event loop and a real socket."""
673+
674+
def test_del_unregisters_fd_from_the_selector(self):
675+
loop = asyncio.SelectorEventLoop()
676+
self.set_event_loop(loop)
677+
rsock, wsock = socket.socketpair()
678+
self.addCleanup(wsock.close)
679+
self.addCleanup(rsock.close)
680+
681+
protocol = test_utils.make_test_protocol(asyncio.Protocol)
682+
tr = _SelectorSocketTransport(loop, rsock, protocol)
683+
test_utils.run_briefly(loop) # let connection_made() and _add_reader()
684+
fd = tr._sock_fd
685+
self.assertIn(fd, loop._selector.get_map())
686+
687+
with self.assertWarns(ResourceWarning):
688+
tr.__del__()
689+
690+
# Leaving the fd registered here would make the loop poll a
691+
# descriptor owned by whatever opens a file next.
692+
self.assertNotIn(fd, loop._selector.get_map())
693+
self.assertEqual(-1, tr._sock_fd)
694+
695+
602696
class SelectorSocketTransportTests(test_utils.TestCase):
603697

604698
def setUp(self):
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Fix :mod:`asyncio` selector transports unregistering a file descriptor they
2+
no longer own. ``_SelectorTransport.__del__`` closed the socket but left the
3+
descriptor registered with the event loop, so a transport resurrected during
4+
garbage collection could later remove an unrelated descriptor from the
5+
selector. ``repr()`` of a closed transport now reports ``fd=-1``.

0 commit comments

Comments
 (0)