Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ def websocket_message(self, flow: http.HTTPFlow, config: dict):
"normal", DEFAULT_SCENARIOS["normal"],
))

task = asyncio.ensure_future(
task = asyncio.create_task(
self._push_telemetry(
flow, scenario, interval_ms / 1000.0,
)
Expand Down Expand Up @@ -169,7 +169,7 @@ def websocket_message(self, flow: http.HTTPFlow, config: dict):
new_scenario, DEFAULT_SCENARIOS["normal"],
))
interval_ms = config.get("push_interval_ms", 100)
task = asyncio.ensure_future(
task = asyncio.create_task(
self._push_telemetry(
flow, scenario, interval_ms / 1000.0,
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,14 +198,14 @@ async def _run_inline_shell_script(
)

# Create a task to monitor the process timeout
start_time = asyncio.get_event_loop().time()
start_time = asyncio.get_running_loop().time()

if timeout is None:
timeout = self.timeout

# Read output in real-time
while process.returncode is None:
if asyncio.get_event_loop().time() - start_time > timeout:
if asyncio.get_running_loop().time() - start_time > timeout:
# Send SIGTERM to entire process group for graceful termination
try:
os.killpg(process.pid, signal.SIGTERM)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class Tftp(Driver):
_shutdown_event: threading.Event = field(init=False, default_factory=threading.Event)
_loop_ready: threading.Event = field(init=False, default_factory=threading.Event)
_loop: Optional[asyncio.AbstractEventLoop] = field(init=False, default=None)
_startup_error: Optional[BaseException] = field(init=False, default=None)

def __post_init__(self):
if hasattr(super(), "__post_init__"):
Expand All @@ -67,28 +68,41 @@ def client(cls) -> str:
return "jumpstarter_driver_tftp.client.TftpServerClient"

def _start_server(self):
self._loop = asyncio.new_event_loop()
asyncio.set_event_loop(self._loop)
self.server = TftpServer(
host=self.host,
port=self.port,
operator=self.children["storage"]._operator,
logger=self.logger,
)
try:
self._loop_ready.set()
self._loop.run_until_complete(self._run_server())
asyncio.run(self._run_server_lifecycle())
except Exception as e:
self.logger.error(f"Error running TFTP server: {e}")
finally:
try:
self._loop.run_until_complete(self._loop.shutdown_asyncgens())
self._loop.close()
except Exception as e:
self.logger.error(f"Error during event loop cleanup: {e}")
self._loop = None
self.logger.info("TFTP server thread completed")

async def _run_server_lifecycle(self):
"""Set up and run the TFTP server within a proper async context.

Uses asyncio.run() instead of the deprecated new_event_loop() +
set_event_loop() + run_until_complete() pattern, which emits
DeprecationWarning on Python 3.12/3.13 and breaks on 3.14
(get_event_loop() raises RuntimeError when no loop is running).
Comment on lines +80 to +84

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed mmahut's review: the previous docstring incorrectly claimed asyncio.Event() needs a running loop on Python 3.14+. Verified from CPython 3.14 source that Event.__init__() does no loop access — the loop is bound lazily via _LoopBoundMixin._get_loop() on first await.

Updated the docstring to state the correct motivation: replacing the deprecated new_event_loop() + set_event_loop() + run_until_complete() pattern with asyncio.run().

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for confirming, this was just a nitpick comment really!

"""
try:
self._loop = asyncio.get_running_loop()
self.server = TftpServer(
host=self.host,
port=self.port,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed CodeRabbit's suggestion: setup code is now wrapped in try/except/finally so _loop_ready.set() always fires. If TftpServer() construction fails, the error is captured in _startup_error and start() re-raises it as a TftpError with the real cause chained — no more waiting 5s for a generic timeout.

Added test_tftp_start_surfaces_startup_error to cover this path.

operator=self.children["storage"]._operator,
logger=self.logger,
)
except Exception as e:
self._startup_error = e
raise
finally:
# Always unblock start() so it can check _startup_error
# instead of waiting for the full timeout.
self._loop_ready.set()
try:
await self._run_server()
finally:
self._loop = None
Comment thread
coderabbitai[bot] marked this conversation as resolved.

async def _run_server(self):
try:
server_task = asyncio.create_task(self.server.start())
Expand Down Expand Up @@ -120,6 +134,7 @@ def start(self):

self._shutdown_event.clear()
self._loop_ready.clear()
self._startup_error = None

self.server_thread = threading.Thread(target=self._start_server, daemon=True)
self.server_thread.start()
Expand All @@ -129,6 +144,10 @@ def start(self):
self.server_thread = None
raise TftpError("Failed to start TFTP server - event loop initialization timeout")

if self._startup_error is not None:
self.server_thread = None
raise TftpError(f"Failed to start TFTP server: {self._startup_error}") from self._startup_error

self.logger.info(f"TFTP server started on {self.host}:{self.port}")

@export
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
from unittest.mock import AsyncMock, patch

import pytest

from jumpstarter_driver_tftp.driver import Tftp
from jumpstarter_driver_tftp.driver import Tftp, TftpError

from jumpstarter.common.utils import serve

Expand Down Expand Up @@ -44,3 +46,79 @@ def test_tftp_root_directory_creation(tmp_path):
server = Tftp(root_dir=str(new_dir))
assert new_dir.exists()
server.close()


def test_tftp_start_stop(tmp_path):
"""Test that start/stop lifecycle works via asyncio.run() in the server thread."""
server = Tftp(root_dir=str(tmp_path), host="127.0.0.1", port=0)
server.start()
try:
# _run_server_lifecycle ran: loop was captured and server was created
assert server.server is not None
assert server._loop is not None
finally:
server.stop()
server.close()


def test_tftp_start_stop_cleans_up_loop(tmp_path):
"""Test that _loop is set to None after shutdown."""
server = Tftp(root_dir=str(tmp_path), host="127.0.0.1", port=0)
server.start()
server.stop()
# After stop, the thread has exited and _loop should be cleaned up
assert server._loop is None
server.close()


def test_tftp_start_server_logs_error_on_failure(tmp_path):
"""Test the error handling path in _start_server."""
server = Tftp(root_dir=str(tmp_path), host="127.0.0.1", port=0)

with patch.object(server, "_run_server_lifecycle", new_callable=AsyncMock, side_effect=RuntimeError("boom")):
# _start_server runs in the calling thread here (not via start())
server._start_server()

# Should not raise, error is logged
server.close()


@pytest.mark.anyio
async def test_tftp_run_server_lifecycle_creates_server_in_async_context(tmp_path):
"""Test that _run_server_lifecycle creates TftpServer within asyncio.run().

This validates the replacement of the deprecated new_event_loop() +
set_event_loop() + run_until_complete() pattern with asyncio.run().
"""
server = Tftp(root_dir=str(tmp_path), host="127.0.0.1", port=0)

# Patch _run_server so we don't actually start listening
with patch.object(server, "_run_server", new_callable=AsyncMock):
await server._run_server_lifecycle()

# Server was created in async context
assert server.server is not None
assert server.server.shutdown_event is not None
assert server.server.ready_event is not None
# Loop ref is cleaned up in the finally block
assert server._loop is None
server.close()


def test_tftp_start_surfaces_startup_error(tmp_path):
"""Test that start() surfaces the real error instead of a generic timeout.

If TftpServer construction fails inside _run_server_lifecycle, the
_startup_error field is set and _loop_ready is signalled via finally,
so start() returns promptly and raises the actual cause.
"""
server = Tftp(root_dir=str(tmp_path), host="127.0.0.1", port=0)

with patch(
"jumpstarter_driver_tftp.driver.TftpServer",
side_effect=RuntimeError("port already in use"),
):
with pytest.raises(TftpError, match="port already in use"):
server.start()

server.close()
4 changes: 4 additions & 0 deletions python/packages/jumpstarter-driver-tftp/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ log_cli = true
log_cli_level = "INFO"
testpaths = ["jumpstarter_driver_tftp"]
asyncio_mode = "auto"
addopts = "--cov --cov-report=html --cov-report=xml"

[tool.coverage.run]
source = ["."]

[build-system]
requires = ["hatchling", "hatch-vcs", "hatch-pin-jumpstarter"]
Expand Down
Loading