Skip to content

Commit f843371

Browse files
committed
gh-156920: fix ProactorEventLoop datagram transports drop buffered datagrams on close() and never call connection_lost()
1 parent d57cb23 commit f843371

3 files changed

Lines changed: 131 additions & 10 deletions

File tree

Lib/asyncio/proactor_events.py

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,9 @@ def close(self):
105105
if self._closing:
106106
return
107107
self._closing = True
108-
self._conn_lost += 1
109108
if not self._buffer and self._write_fut is None:
109+
# Nothing left to flush: no more data will be sent.
110+
self._conn_lost += 1
110111
self._loop.call_soon(self._call_connection_lost, None)
111112
if self._read_fut is not None:
112113
self._read_fut.cancel()
@@ -386,6 +387,7 @@ def _loop_writing(self, f=None, data=None):
386387
self._buffer = None
387388
if not data:
388389
if self._closing:
390+
self._conn_lost += 1
389391
self._loop.call_soon(self._call_connection_lost, None)
390392
if self._eof_written:
391393
self._sock.shutdown(socket.SHUT_WR)
@@ -480,6 +482,11 @@ def get_write_buffer_size(self):
480482
def abort(self):
481483
self._force_close(None)
482484

485+
def _force_close(self, exc):
486+
# The base class drops the buffer; the size is tracked separately.
487+
self._buffer_size = 0
488+
super()._force_close(exc)
489+
483490
def sendto(self, data, addr=None):
484491
if not isinstance(data, (bytes, bytearray, memoryview)):
485492
raise TypeError('data argument must be bytes-like object (%r)',
@@ -509,6 +516,8 @@ def sendto(self, data, addr=None):
509516
def _loop_writing(self, fut=None):
510517
try:
511518
if self._conn_lost:
519+
# No more data will be sent: either everything buffered has
520+
# already been flushed, or _force_close() dropped it.
512521
return
513522

514523
assert fut is self._write_fut
@@ -517,9 +526,10 @@ def _loop_writing(self, fut=None):
517526
# We are in a _loop_writing() done callback, get the result
518527
fut.result()
519528

520-
if not self._buffer or (self._conn_lost and self._address):
521-
# The connection has been closed
529+
if not self._buffer:
530+
# Everything buffered has been sent
522531
if self._closing:
532+
self._conn_lost += 1
523533
self._loop.call_soon(self._call_connection_lost, None)
524534
return
525535

@@ -534,17 +544,26 @@ def _loop_writing(self, fut=None):
534544
addr=addr)
535545
except OSError as exc:
536546
self._protocol.error_received(exc)
537-
if self._buffer:
538-
# Re-arm the write loop so buffered data isn't stranded and
539-
# a paused protocol is eventually resumed (gh-156698).
540-
def resume_writing():
541-
# a sendto() may have armed a write in the meantime;
542-
# its own callback will drain the rest of the buffer.
547+
# error_received() is arbitrary protocol code: it may have sent
548+
# (arming a write of its own, directly or via call_soon()),
549+
# closed, or aborted the transport.
550+
if self._buffer or self._closing:
551+
# Either data is still queued, or a close() is waiting on
552+
# the write loop to drain it and call connection_lost().
553+
# This write failed, so there is no completion callback
554+
# pending to re-enter the loop -- schedule one (gh-156698).
555+
def write_next():
556+
# error_received() may have armed a write of its own,
557+
# directly or with call_soon(); its completion callback
558+
# will drain the rest of the buffer.
543559
if self._write_fut is None:
544560
self._loop_writing()
545561

546-
self._loop.call_soon(resume_writing)
562+
self._loop.call_soon(write_next)
547563
else:
564+
# Nothing left to write, so a paused protocol has to be
565+
# resumed here: the next entry into _loop_writing() returns
566+
# early on an empty buffer without doing it.
548567
self._maybe_resume_protocol()
549568
except Exception as exc:
550569
self._fatal_error(exc, 'Fatal write error on datagram transport')

Lib/test/test_asyncio/test_events.py

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1712,6 +1712,103 @@ def error_received(self, exc):
17121712
unhandled,
17131713
f'unhandled exception in the write loop: {unhandled}')
17141714

1715+
def test_datagram_close_flushes_queued_data(self):
1716+
# See https://github.com/python/cpython/issues/156920: _conn_lost
1717+
# used to mean "close() was requested" rather than "no more data
1718+
# will be sent". Since add_done_callback() always defers an
1719+
# already-completed write's callback with call_soon(), a sendto()
1720+
# immediately followed by close() -- with no await in between --
1721+
# leaves a write genuinely outstanding at close() time on every
1722+
# platform, not just a slow one. Closing must let that write (and
1723+
# anything queued behind it) drain and still call connection_lost(),
1724+
# instead of tripping the "no more data will be sent" guard before
1725+
# the drain has actually happened and hanging forever.
1726+
loop = self.loop
1727+
1728+
class Receiver(asyncio.DatagramProtocol):
1729+
def connection_made(self, transport):
1730+
self.received = []
1731+
1732+
def datagram_received(self, data, addr):
1733+
self.received.append(data)
1734+
1735+
recv_sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
1736+
recv_sock.setblocking(False)
1737+
recv_sock.bind(('127.0.0.1', 0))
1738+
recv_transport, receiver = loop.run_until_complete(
1739+
loop.create_datagram_endpoint(Receiver, sock=recv_sock))
1740+
addr = recv_sock.getsockname()
1741+
1742+
class Protocol(asyncio.DatagramProtocol):
1743+
def connection_made(self, transport):
1744+
self.lost = loop.create_future()
1745+
1746+
def connection_lost(self, exc):
1747+
if not self.lost.done():
1748+
self.lost.set_result(exc)
1749+
1750+
sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
1751+
sock.setblocking(False)
1752+
sock.bind(('127.0.0.1', 0))
1753+
transport, protocol = loop.run_until_complete(
1754+
loop.create_datagram_endpoint(Protocol, sock=sock))
1755+
1756+
# 'first' is still in flight (its completion callback hasn't run
1757+
# yet) and 'second' is queued behind it when close() is called.
1758+
transport.sendto(b'first', addr)
1759+
transport.sendto(b'second', addr)
1760+
transport.close()
1761+
1762+
loop.run_until_complete(asyncio.wait_for(protocol.lost, 10))
1763+
1764+
test_utils.run_until(
1765+
loop, lambda: len(receiver.received) >= 2)
1766+
self.assertEqual(sorted(receiver.received), [b'first', b'second'])
1767+
1768+
recv_transport.close()
1769+
test_utils.run_briefly(loop)
1770+
1771+
def test_datagram_close_during_write_error_calls_connection_lost(self):
1772+
# See https://github.com/python/cpython/issues/156920: if the
1773+
# write that's outstanding when close() is called goes on to fail
1774+
# (rather than succeed), the failure handler used to only re-arm
1775+
# the write loop when data was still queued behind it. If that
1776+
# failing write was the last thing in the buffer, nothing re-armed
1777+
# the loop, so the close() in progress never got to call
1778+
# connection_lost() -- it hung forever instead of finishing once
1779+
# the buffer was actually empty.
1780+
loop = self.loop
1781+
1782+
class Protocol(asyncio.DatagramProtocol):
1783+
def connection_made(self, transport):
1784+
self.lost = loop.create_future()
1785+
self.errors = []
1786+
1787+
def error_received(self, exc):
1788+
self.errors.append(exc)
1789+
1790+
def connection_lost(self, exc):
1791+
if not self.lost.done():
1792+
self.lost.set_result(exc)
1793+
1794+
sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
1795+
sock.setblocking(False)
1796+
sock.bind(('127.0.0.1', 0))
1797+
transport, protocol = loop.run_until_complete(
1798+
loop.create_datagram_endpoint(Protocol, sock=sock))
1799+
addr = sock.getsockname()
1800+
1801+
# 'ok' is still in flight when close() is called; 'oversized' is
1802+
# queued behind it and fails once it reaches the front of the
1803+
# buffer, leaving the buffer empty right as the error is handled.
1804+
oversized = b'\x00' * 70000
1805+
transport.sendto(b'ok', addr)
1806+
transport.sendto(oversized, addr)
1807+
transport.close()
1808+
1809+
loop.run_until_complete(asyncio.wait_for(protocol.lost, 10))
1810+
self.assertTrue(protocol.errors)
1811+
17151812
def test_internal_fds(self):
17161813
loop = self.create_event_loop()
17171814
if not isinstance(loop, selector_events.BaseSelectorEventLoop):
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Fix :mod:`asyncio` on Windows: closing a :class:`~asyncio.DatagramTransport`
2+
under :class:`~asyncio.ProactorEventLoop` while datagrams were still queued,
3+
or while an in-flight write failed right as ``close()`` was draining the
4+
buffer, could strand the queued data and never call ``connection_lost()``,
5+
hanging the close indefinitely.

0 commit comments

Comments
 (0)