Skip to content

Commit 21c01c8

Browse files
ccie18643claude
andcommitted
fix(dhcp4): make the DHCPv4 client INIT path responsive to stop()
'pytcp stack start' could not be interrupted: a Ctrl-C ran the full teardown, but the DHCPv4 client worker — parked in the RFC 2131 §4.4.1 startup desync 'time.sleep' (or a multi-second blocking recv in the retransmission backoff) — missed the bounded 2.0 s join in 'Subsystem.stop()' and was left dangling. Because the worker was a non-daemon thread, it then kept the interpreter alive AND ran the entire acquisition (DISCOVER + 5 retransmits, ~2 min against a silent server), so further Ctrl-Cs did nothing. Two fixes: - DHCPv4 client: the desync delay and the post-DECLINE backoff now wait on the stop event instead of 'time.sleep' (interruptible; in sync 'fetch()' the event is never set, so they still behave as a sleep), '_do_init_to_bound' aborts before opening a socket / before each NAK-restart when stop is set, and '_recv_with_backoff' bails between attempts. A daemon-mode stop() during INIT now halts immediately without further wire traffic. - Subsystem: spawn the worker as a daemon thread. The bounded join in stop() still gives a graceful exit; the daemon flag is the safety net so a worker stuck in a syscall is abandoned at interpreter exit rather than wedging the process. Tests-first. New TestDhcp4ClientStopResponsive pins: abort-before-socket on stop, recv-backoff immediate bail, interruptible desync wait, and a real-thread daemon stop() during the desync delay that does not leave the worker dangling. New test__subsystem__worker_thread_is_daemon pins the daemon flag. The three initial-delay tests and the decline-backoff test are updated to assert the interruptible stop-event wait rather than 'time.sleep'. Reference: RFC 2131 §4.4.1 (startup desynchronisation delay). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 8e4c209 commit 21c01c8

4 files changed

Lines changed: 212 additions & 21 deletions

File tree

packages/pytcp/pytcp/protocols/dhcp4/dhcp4__client.py

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1351,6 +1351,10 @@ def _do_init_to_bound(self) -> Dhcp4Lease | None:
13511351
"""
13521352

13531353
self._initial_delay()
1354+
# A daemon-mode 'stop()' during the desync delay returns here
1355+
# without opening a socket or emitting any wire traffic.
1356+
if self._event__stop_subsystem.is_set():
1357+
return None
13541358
self._fetch_started_at_monotonic = time.monotonic()
13551359
__debug__ and log(
13561360
"dhcp4",
@@ -1363,6 +1367,8 @@ def _do_init_to_bound(self) -> Dhcp4Lease | None:
13631367
client_socket.connect(("255.255.255.255", 67))
13641368

13651369
for _ in range(dhcp4__constants.DHCP4__NAK_MAX_RESTARTS + 1):
1370+
if self._event__stop_subsystem.is_set():
1371+
return None
13661372
outcome = self._discover_request_once(client_socket)
13671373
if not isinstance(outcome, _NakRestart):
13681374
if isinstance(outcome, Dhcp4Lease):
@@ -1484,7 +1490,9 @@ def _discover_request_once(self, client_socket: socket) -> Dhcp4Lease | _NakRest
14841490
)
14851491
backoff_s = dhcp4__constants.DHCP4__DECLINE_BACKOFF_MS / 1000.0
14861492
if backoff_s > 0:
1487-
time.sleep(backoff_s)
1493+
# Interruptible wait so a daemon-mode 'stop()' during the
1494+
# post-DECLINE backoff is not wedged for the full window.
1495+
self._event__stop_subsystem.wait(timeout=backoff_s)
14881496
return _NAK_RESTART
14891497

14901498
t1_override, t2_override = self._extract_t1_t2_overrides(ack, ack.lease_time)
@@ -1529,6 +1537,12 @@ def _recv_with_backoff(
15291537
jitter_ms = dhcp4__constants.DHCP4__RETRANS_JITTER_MS
15301538

15311539
for attempt in range(max_attempts):
1540+
# Bail between attempts so a daemon-mode 'stop()' halts the
1541+
# retransmission storm promptly instead of running the full
1542+
# backoff budget (which can span minutes against a silent
1543+
# server). The event is never set in sync 'fetch()'.
1544+
if self._event__stop_subsystem.is_set():
1545+
return None
15321546
jitter_s = random.uniform(-jitter_ms / 1000.0, jitter_ms / 1000.0)
15331547
timeout_s = max(0.001, delay_ms / 1000.0 + jitter_s)
15341548
result = self._recv_within_window(
@@ -1828,7 +1842,11 @@ def _initial_delay(self) -> None:
18281842
min_ms = dhcp4__constants.DHCP4__INIT_DELAY_MIN_MS
18291843
delay_s = random.uniform(min_ms / 1000.0, max_ms / 1000.0)
18301844
__debug__ and log("dhcp4", f"Initial desync delay: {delay_s:.2f}s")
1831-
time.sleep(delay_s)
1845+
# Interruptible wait, not 'time.sleep': in daemon mode a 'stop()'
1846+
# during the desync window must wake the worker immediately so
1847+
# 'stack.stop()' is not wedged for the full delay. In sync
1848+
# 'fetch()' the event is never set, so this behaves as a sleep.
1849+
self._event__stop_subsystem.wait(timeout=delay_s)
18321850

18331851
def _elapsed_secs(self) -> int:
18341852
"""

packages/pytcp/pytcp/runtime/subsystem.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,13 @@ def start(self) -> None:
109109
)
110110

111111
self._event__stop_subsystem.clear()
112-
self._thread = threading.Thread(target=self._thread__subsystem)
112+
# Daemon worker: 'stop()' joins it with a bounded 2.0 s timeout
113+
# for a graceful exit, but a subclass blocked in a syscall (a
114+
# DHCPv4 client mid-recv, a ring blocked on the TAP fd) can miss
115+
# that window and be left dangling. A daemon thread is then
116+
# abandoned at interpreter exit instead of wedging the process —
117+
# the safety net behind the bounded join in 'stop()'.
118+
self._thread = threading.Thread(target=self._thread__subsystem, daemon=True)
113119
self._thread.start()
114120
self._start()
115121

packages/pytcp/pytcp/tests/unit/protocols/dhcp4/test__dhcp4__client.py

Lines changed: 158 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
"""
3232

3333
import errno
34+
import threading
3435
from typing import Any, cast, override
3536
from unittest import TestCase
3637
from unittest.mock import MagicMock, create_autospec, patch
@@ -1156,14 +1157,20 @@ def test__dhcp4_client__fetch_initial_delay_uses_default_bounds(self) -> None:
11561157
self._server.enqueue_offer()
11571158
self._server.enqueue_ack()
11581159

1160+
client = Dhcp4Client(mac_address=_DEFAULT_MAC)
11591161
with (
11601162
patch("pytcp.protocols.dhcp4.dhcp4__client.random.uniform", return_value=4.2) as mock_uniform,
1163+
patch.object(client._event__stop_subsystem, "wait") as mock_wait,
11611164
patch("pytcp.protocols.dhcp4.dhcp4__client.time.sleep") as mock_sleep,
11621165
):
1163-
Dhcp4Client(mac_address=_DEFAULT_MAC).fetch()
1166+
client.fetch()
11641167

11651168
mock_uniform.assert_any_call(1.0, 10.0)
1166-
mock_sleep.assert_any_call(4.2)
1169+
# The desync delay is an interruptible wait on the stop event, not
1170+
# an uninterruptible 'time.sleep', so a daemon-mode stop() during
1171+
# the window wakes it immediately.
1172+
mock_wait.assert_any_call(timeout=4.2)
1173+
mock_sleep.assert_not_called()
11671174

11681175
def test__dhcp4_client__fetch_initial_delay_honours_custom_sysctl_bounds(self) -> None:
11691176
"""
@@ -1179,11 +1186,12 @@ def test__dhcp4_client__fetch_initial_delay_honours_custom_sysctl_bounds(self) -
11791186
self._server.enqueue_offer()
11801187
self._server.enqueue_ack()
11811188

1189+
client = Dhcp4Client(mac_address=_DEFAULT_MAC)
11821190
with (
11831191
patch("pytcp.protocols.dhcp4.dhcp4__client.random.uniform", return_value=1.0) as mock_uniform,
1184-
patch("pytcp.protocols.dhcp4.dhcp4__client.time.sleep"),
1192+
patch.object(client._event__stop_subsystem, "wait"),
11851193
):
1186-
Dhcp4Client(mac_address=_DEFAULT_MAC).fetch()
1194+
client.fetch()
11871195

11881196
# 500 ms / 1000 = 0.5 s; 2500 ms / 1000 = 2.5 s.
11891197
mock_uniform.assert_any_call(0.5, 2.5)
@@ -1193,24 +1201,153 @@ def test__dhcp4_client__fetch_initial_delay_disabled_when_max_ms_zero(self) -> N
11931201
Ensure the startup desync delay is bypassed entirely when
11941202
'dhcp.init_delay_max_ms' is 0 — the canonical
11951203
disable-for-tests configuration that the fixture base
1196-
applies by default. 'time.sleep' must not be invoked from
1197-
the initial-delay path on the happy-path 'fetch()'.
1204+
applies by default. The interruptible stop-event wait must
1205+
not be invoked from the initial-delay path on the happy-path
1206+
'fetch()'.
11981207
11991208
Reference: PyTCP test infrastructure (no RFC clause).
12001209
"""
12011210

12021211
# Fixture base already sets both bounds to 0; no override
12031212
# needed here. The Phase 1 backoff path uses 'recv__mv(timeout=)'
1204-
# rather than 'time.sleep', so 'time.sleep' should not be
1205-
# called at all during a happy-path fetch.
1213+
# rather than the stop-event wait, so the desync wait should not
1214+
# be called at all during a happy-path fetch.
12061215
self._server.enqueue_offer()
12071216
self._server.enqueue_ack()
12081217

1209-
with patch("pytcp.protocols.dhcp4.dhcp4__client.time.sleep") as mock_sleep:
1210-
Dhcp4Client(mac_address=_DEFAULT_MAC).fetch()
1218+
client = Dhcp4Client(mac_address=_DEFAULT_MAC)
1219+
with patch.object(client._event__stop_subsystem, "wait") as mock_wait:
1220+
client.fetch()
1221+
1222+
mock_wait.assert_not_called()
1223+
1224+
1225+
class TestDhcp4ClientStopResponsive(_Dhcp4ClientFixture):
1226+
"""
1227+
The daemon-mode stop-responsiveness tests — the INIT acquisition
1228+
path must observe the stop event promptly so 'stack.stop()' is not
1229+
wedged by a DHCPv4 client mid-acquisition.
1230+
"""
1231+
1232+
def test__dhcp4_client__do_init_to_bound_aborts_before_socket_when_stop_set(self) -> None:
1233+
"""
1234+
Ensure '_do_init_to_bound' returns None without opening a client
1235+
socket when the stop event is already set, so a stop() arriving
1236+
during the startup desync window does not proceed to a wire
1237+
exchange.
1238+
1239+
Reference: PyTCP test infrastructure (no RFC clause).
1240+
"""
1241+
1242+
client = Dhcp4Client(mac_address=_DEFAULT_MAC)
1243+
client._event__stop_subsystem.set()
1244+
1245+
result = client._do_init_to_bound()
1246+
1247+
self.assertIsNone(
1248+
result,
1249+
msg="A stop-signalled INIT must abandon acquisition and return None.",
1250+
)
1251+
self._socket_factory.assert_not_called()
1252+
1253+
def test__dhcp4_client__recv_backoff_bails_immediately_when_stop_set(self) -> None:
1254+
"""
1255+
Ensure '_recv_with_backoff' returns None without issuing any
1256+
recv or invoking the resend callback when the stop event is
1257+
already set, so the retransmission storm halts the instant
1258+
stop() is requested.
1259+
1260+
Reference: PyTCP test infrastructure (no RFC clause).
1261+
"""
1262+
1263+
client = Dhcp4Client(mac_address=_DEFAULT_MAC)
1264+
client._event__stop_subsystem.set()
1265+
resend = MagicMock()
1266+
1267+
result = client._recv_with_backoff(
1268+
self._sock,
1269+
expected_type=Dhcp4MessageType.ACK,
1270+
xid=_PINNED_XID,
1271+
resend=resend,
1272+
)
12111273

1274+
self.assertIsNone(
1275+
result,
1276+
msg="A stop-signalled recv backoff must return None without waiting.",
1277+
)
1278+
self._sock.recv__mv.assert_not_called()
1279+
resend.assert_not_called()
1280+
1281+
def test__dhcp4_client__initial_delay_waits_on_interruptible_stop_event(self) -> None:
1282+
"""
1283+
Ensure the startup desync delay waits on the stop event (so it
1284+
is interruptible) rather than calling the uninterruptible
1285+
'time.sleep'.
1286+
1287+
Reference: RFC 2131 §4.4.1 (random startup desync delay).
1288+
"""
1289+
1290+
self.enterContext(sysctl.override("dhcp.init_delay_min_ms", 1000))
1291+
self.enterContext(sysctl.override("dhcp.init_delay_max_ms", 10000))
1292+
self._server.enqueue_offer()
1293+
self._server.enqueue_ack()
1294+
1295+
client = Dhcp4Client(mac_address=_DEFAULT_MAC)
1296+
with (
1297+
patch("pytcp.protocols.dhcp4.dhcp4__client.random.uniform", return_value=3.0),
1298+
patch.object(client._event__stop_subsystem, "wait") as mock_wait,
1299+
patch("pytcp.protocols.dhcp4.dhcp4__client.time.sleep") as mock_sleep,
1300+
):
1301+
client.fetch()
1302+
1303+
mock_wait.assert_any_call(timeout=3.0)
12121304
mock_sleep.assert_not_called()
12131305

1306+
def test__dhcp4_client__daemon_stop_during_init_delay_does_not_dangle(self) -> None:
1307+
"""
1308+
Ensure a daemon-mode 'stop()' issued while the worker is parked
1309+
in the startup desync delay terminates the worker thread instead
1310+
of leaving it dangling for the full delay (the wedge that made
1311+
'pytcp stack start' unstoppable).
1312+
1313+
Reference: PyTCP test infrastructure (no RFC clause).
1314+
"""
1315+
1316+
self.enterContext(sysctl.override("dhcp.init_delay_min_ms", 5000))
1317+
self.enterContext(sysctl.override("dhcp.init_delay_max_ms", 5000))
1318+
self.enterContext(patch("pytcp.runtime.subsystem.log"))
1319+
1320+
reached_delay = threading.Event()
1321+
1322+
def _uniform(*args: float) -> float:
1323+
del args
1324+
reached_delay.set()
1325+
return 5.0
1326+
1327+
client = Dhcp4Client(mac_address=_DEFAULT_MAC)
1328+
# Captured the instant stop() returns (after its bounded 2.0 s
1329+
# join) — BEFORE the cleanup join below, which would otherwise
1330+
# mask a wedged worker by waiting out the full desync delay.
1331+
alive_after_stop = True
1332+
with patch("pytcp.protocols.dhcp4.dhcp4__client.random.uniform", side_effect=_uniform):
1333+
client.start()
1334+
try:
1335+
self.assertTrue(
1336+
reached_delay.wait(timeout=5.0),
1337+
msg="Precondition: the worker must reach the desync delay.",
1338+
)
1339+
client.stop()
1340+
assert client._thread is not None
1341+
alive_after_stop = client._thread.is_alive()
1342+
finally:
1343+
if client._thread is not None:
1344+
client._thread.join(timeout=10.0)
1345+
1346+
self.assertFalse(
1347+
alive_after_stop,
1348+
msg="stop() during the desync delay must terminate the worker promptly.",
1349+
)
1350+
12141351

12151352
class TestDhcp4ClientFetchArpDad(_Dhcp4ClientFixture):
12161353
"""
@@ -1415,15 +1552,18 @@ def test__dhcp4_client__fetch_decline_path_honours_decline_backoff_sleep(self) -
14151552

14161553
acd = _acd_mock(probe_results=False, conflict_mac=MacAddress("02:00:00:00:00:99"))
14171554

1418-
with patch("pytcp.protocols.dhcp4.dhcp4__client.time.sleep") as mock_sleep:
1419-
Dhcp4Client(
1420-
mac_address=_DEFAULT_MAC,
1421-
acd=acd,
1422-
).fetch()
1555+
client = Dhcp4Client(mac_address=_DEFAULT_MAC, acd=acd)
1556+
with (
1557+
patch.object(client._event__stop_subsystem, "wait") as mock_wait,
1558+
patch("pytcp.protocols.dhcp4.dhcp4__client.time.sleep") as mock_sleep,
1559+
):
1560+
client.fetch()
14231561

1424-
# Initial-delay sleep is disabled by the fixture; only the
1425-
# decline-backoff sleep should fire.
1426-
mock_sleep.assert_called_once_with(5.0)
1562+
# Initial-delay wait is disabled by the fixture; only the
1563+
# decline-backoff wait should fire — and as an interruptible
1564+
# stop-event wait, not an uninterruptible 'time.sleep'.
1565+
mock_wait.assert_called_once_with(timeout=5.0)
1566+
mock_sleep.assert_not_called()
14271567

14281568

14291569
class TestDhcp4ClientFetchRfc4361Cid(_Dhcp4ClientFixture):

packages/pytcp/pytcp/tests/unit/runtime/test__runtime__subsystem.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,33 @@ def test__subsystem__start_runs_thread_and_fires_hooks(self) -> None:
329329
finally:
330330
subsystem.stop()
331331

332+
def test__subsystem__worker_thread_is_daemon(self) -> None:
333+
"""
334+
Ensure 'start()' spawns the worker as a daemon thread so a
335+
subsystem blocked in a syscall (e.g. a DHCPv4 client mid-recv)
336+
can never wedge interpreter exit after the bounded join in
337+
'stop()' times out and leaves it dangling.
338+
339+
Reference: PyTCP test infrastructure (no RFC clause).
340+
"""
341+
342+
subsystem = _TestSubsystem()
343+
344+
try:
345+
subsystem.start()
346+
347+
self.assertTrue(
348+
subsystem._loop_event.wait(timeout=2.0),
349+
msg="Precondition: the worker thread must be running.",
350+
)
351+
assert subsystem._thread is not None
352+
self.assertTrue(
353+
subsystem._thread.daemon,
354+
msg="Subsystem.start() must spawn the worker as a daemon thread.",
355+
)
356+
finally:
357+
subsystem.stop()
358+
332359
def test__subsystem__stop_signals_event_and_fires_hook(self) -> None:
333360
"""
334361
Ensure 'stop()' sets the stop event (terminating the loop) and

0 commit comments

Comments
 (0)