Move z-machine restore before interpreter thread start
Replaces the async _do_restore() (called after thread launch) with a synchronous _try_restore() called before the thread starts. This eliminates the race condition where restore mutates z-machine state while the interpreter thread is running. The restore prefix message is now part of start()'s return value instead of being sent separately in play.py.
This commit is contained in:
parent
224c1f0645
commit
15e1d807aa
GPG key ID:
22B5C6D66A38B06C
4 changed files with 126 additions and 67 deletions
|
|
@ -91,18 +91,9 @@ async def cmd_play(player: Player, args: str) -> None:
|
|||
|
||||
await player.send("(type ::help for escape commands)\r\n")
|
||||
|
||||
# Check for saved game (both session types now support _do_restore)
|
||||
if hasattr(session, "_do_restore") and session.save_path.exists():
|
||||
await player.send("restoring saved game...\r\n")
|
||||
restored_text = await session._do_restore()
|
||||
if restored_text:
|
||||
await player.send(restored_text + "\r\n")
|
||||
# Broadcast restored text to spectators
|
||||
spectator_msg = f"[{player.name}'s terminal]\r\n{restored_text}\r\n"
|
||||
await broadcast_to_spectators(player, spectator_msg)
|
||||
elif intro:
|
||||
if intro:
|
||||
await player.send(intro + "\r\n")
|
||||
# Broadcast intro to spectators
|
||||
# Broadcast to spectators
|
||||
spectator_msg = f"[{player.name}'s terminal]\r\n{intro}\r\n"
|
||||
await broadcast_to_spectators(player, spectator_msg)
|
||||
|
||||
|
|
|
|||
|
|
@ -40,15 +40,38 @@ class EmbeddedIFSession:
|
|||
safe_name = re.sub(r"[^a-zA-Z0-9_-]", "_", self.player.name)
|
||||
return self._data_dir / "if_saves" / safe_name / f"{self.game_name}.qzl"
|
||||
|
||||
def _try_restore(self) -> bool:
|
||||
"""Try to restore from save file before interpreter starts.
|
||||
|
||||
Must be called before the interpreter thread is launched.
|
||||
Returns True if state was restored successfully.
|
||||
"""
|
||||
if not self.save_path.exists():
|
||||
return False
|
||||
try:
|
||||
save_data = self.save_path.read_bytes()
|
||||
parser = QuetzalParser(self._zmachine)
|
||||
parser.load_from_bytes(save_data)
|
||||
return True
|
||||
except Exception as e:
|
||||
logger.debug(f"Restore failed: {e}")
|
||||
return False
|
||||
|
||||
async def start(self) -> str:
|
||||
"""Start the z-machine interpreter, restoring from save if available."""
|
||||
restored = self._try_restore()
|
||||
|
||||
self._thread = threading.Thread(target=self._run_interpreter, daemon=True)
|
||||
self._thread.start()
|
||||
|
||||
loop = asyncio.get_running_loop()
|
||||
await loop.run_in_executor(None, self._keyboard._waiting.wait)
|
||||
|
||||
intro = self._screen.flush()
|
||||
return intro
|
||||
output = self._screen.flush()
|
||||
if restored:
|
||||
prefix = "restoring saved game...\r\nrestored."
|
||||
return f"{prefix}\r\n\r\n{output}" if output else prefix
|
||||
return output
|
||||
|
||||
async def handle_input(self, text: str) -> IFResponse:
|
||||
if text.lower() == "::quit":
|
||||
|
|
@ -112,34 +135,6 @@ class EmbeddedIFSession:
|
|||
except Exception as e:
|
||||
return f"error: save failed ({e})"
|
||||
|
||||
async def _do_restore(self) -> str:
|
||||
"""Restore game state from disk. Returns status message."""
|
||||
if not self.save_path.exists():
|
||||
return ""
|
||||
try:
|
||||
save_data = self.save_path.read_bytes()
|
||||
parser = QuetzalParser(self._zmachine)
|
||||
parser.load_from_bytes(save_data)
|
||||
# Flush stale intro text from screen buffer
|
||||
self._screen.flush()
|
||||
# Feed a blank line to continue execution from restored state
|
||||
self._keyboard._waiting.clear()
|
||||
self._keyboard.feed("")
|
||||
# Wait for interpreter to process and generate output
|
||||
loop = asyncio.get_running_loop()
|
||||
|
||||
def wait_for_output():
|
||||
while not self._done and not self._keyboard._waiting.is_set():
|
||||
self._keyboard._waiting.wait(timeout=0.1)
|
||||
|
||||
await loop.run_in_executor(None, wait_for_output)
|
||||
# Get the output from restored state
|
||||
output = self._screen.flush()
|
||||
return f"restored.\r\n{output}" if output else "restored."
|
||||
except Exception as e:
|
||||
logger.debug(f"Restore failed: {e}")
|
||||
return ""
|
||||
|
||||
def get_location_name(self) -> str | None:
|
||||
try:
|
||||
location_obj = self._zmachine._mem.read_global(0)
|
||||
|
|
|
|||
|
|
@ -92,6 +92,10 @@ async def test_embedded_session_start():
|
|||
player = Player(name="tester", x=5, y=5, writer=mock_writer)
|
||||
|
||||
session = EmbeddedIFSession(player, str(ZORK_PATH), "zork1")
|
||||
# Clean up any existing save to get a fresh start
|
||||
if session.save_path.exists():
|
||||
session.save_path.unlink()
|
||||
|
||||
intro = await session.start()
|
||||
|
||||
assert intro is not None
|
||||
|
|
@ -111,6 +115,10 @@ async def test_embedded_session_handle_input():
|
|||
player = Player(name="tester", x=5, y=5, writer=mock_writer)
|
||||
|
||||
session = EmbeddedIFSession(player, str(ZORK_PATH), "zork1")
|
||||
# Clean up any existing save to get a fresh start
|
||||
if session.save_path.exists():
|
||||
session.save_path.unlink()
|
||||
|
||||
await session.start()
|
||||
|
||||
response = await session.handle_input("look")
|
||||
|
|
@ -204,10 +212,83 @@ async def test_embedded_session_room_objects():
|
|||
assert len(objects) >= 0 # May or may not have visible objects initially
|
||||
|
||||
|
||||
@requires_zork
|
||||
@pytest.mark.asyncio
|
||||
async def test_embedded_session_try_restore_before_thread():
|
||||
"""_try_restore() is called synchronously before interpreter thread starts."""
|
||||
from unittest.mock import patch
|
||||
|
||||
from mudlib.embedded_if_session import EmbeddedIFSession
|
||||
from mudlib.player import Player
|
||||
|
||||
mock_writer = MockWriter()
|
||||
player = Player(name="tester", x=5, y=5, writer=mock_writer)
|
||||
session = EmbeddedIFSession(player, str(ZORK_PATH), "zork1")
|
||||
|
||||
# Create a save file
|
||||
if session.save_path.exists():
|
||||
session.save_path.unlink()
|
||||
session.save_path.parent.mkdir(parents=True, exist_ok=True)
|
||||
# Write a minimal valid save (header only, won't actually restore correctly)
|
||||
session.save_path.write_bytes(b"FORM\x00\x00\x00\x08IFZSQUTZ\x00\x00\x00\x00")
|
||||
|
||||
call_order = []
|
||||
|
||||
original_try_restore = session._try_restore
|
||||
original_run_interpreter = session._run_interpreter
|
||||
|
||||
def track_try_restore():
|
||||
call_order.append("try_restore")
|
||||
return original_try_restore()
|
||||
|
||||
def track_run_interpreter():
|
||||
call_order.append("run_interpreter")
|
||||
original_run_interpreter()
|
||||
|
||||
with (
|
||||
patch.object(session, "_try_restore", side_effect=track_try_restore),
|
||||
patch.object(session, "_run_interpreter", side_effect=track_run_interpreter),
|
||||
):
|
||||
await session.start()
|
||||
|
||||
# Verify _try_restore was called before _run_interpreter
|
||||
assert call_order[0] == "try_restore"
|
||||
assert call_order[1] == "run_interpreter"
|
||||
|
||||
await session.stop()
|
||||
if session.save_path.exists():
|
||||
session.save_path.unlink()
|
||||
|
||||
|
||||
@requires_zork
|
||||
@pytest.mark.asyncio
|
||||
async def test_embedded_session_no_restore_without_save():
|
||||
"""start() does not restore when no save file exists."""
|
||||
from mudlib.embedded_if_session import EmbeddedIFSession
|
||||
from mudlib.player import Player
|
||||
|
||||
mock_writer = MockWriter()
|
||||
player = Player(name="nosaveplayer", writer=mock_writer, x=0, y=0)
|
||||
session = EmbeddedIFSession(player, str(ZORK_PATH), "zork1")
|
||||
|
||||
# Ensure no save file exists
|
||||
if session.save_path.exists():
|
||||
session.save_path.unlink()
|
||||
|
||||
intro = await session.start()
|
||||
|
||||
# Should NOT contain restore message
|
||||
assert "restoring" not in intro.lower()
|
||||
# Should contain normal game intro
|
||||
assert "ZORK" in intro or "West of House" in intro
|
||||
|
||||
await session.stop()
|
||||
|
||||
|
||||
@requires_zork
|
||||
@pytest.mark.asyncio
|
||||
async def test_embedded_session_save_and_restore():
|
||||
"""Save a game, create new session, restore it."""
|
||||
"""Save a game, create new session, restore it via start()."""
|
||||
from mudlib.embedded_if_session import EmbeddedIFSession
|
||||
from mudlib.player import Player
|
||||
|
||||
|
|
@ -215,6 +296,10 @@ async def test_embedded_session_save_and_restore():
|
|||
# Start first session
|
||||
player = Player(name="testplayer", writer=mock_writer, x=0, y=0)
|
||||
session = EmbeddedIFSession(player, str(ZORK_PATH), "zork1")
|
||||
# Clean up any existing save to get a fresh start
|
||||
if session.save_path.exists():
|
||||
session.save_path.unlink()
|
||||
|
||||
await session.start()
|
||||
|
||||
# Do something to change state
|
||||
|
|
@ -225,11 +310,13 @@ async def test_embedded_session_save_and_restore():
|
|||
assert "saved" in save_result.output.lower()
|
||||
await session.stop()
|
||||
|
||||
# Start new session - should auto-restore via play.py pattern
|
||||
# Start new session - should auto-restore via start()
|
||||
# start() calls _try_restore() BEFORE launching the interpreter thread
|
||||
session2 = EmbeddedIFSession(player, str(ZORK_PATH), "zork1")
|
||||
await session2.start()
|
||||
result = await session2._do_restore()
|
||||
assert result # Should have restored
|
||||
intro = await session2.start()
|
||||
# Should contain restore message prefixed to output
|
||||
assert "restoring saved game" in intro.lower()
|
||||
assert "restored" in intro.lower()
|
||||
|
||||
# The game state should reflect the restored state
|
||||
# (location may differ after restore, just verify it works)
|
||||
|
|
|
|||
|
|
@ -131,19 +131,16 @@ async def test_play_handles_dfrotz_missing(player):
|
|||
|
||||
@pytest.mark.asyncio
|
||||
async def test_play_restores_save_if_exists(player):
|
||||
"""Playing restores saved game if save file exists."""
|
||||
from pathlib import Path
|
||||
|
||||
"""Playing restores saved game if save file exists (via start())."""
|
||||
from mudlib.commands.play import cmd_play
|
||||
|
||||
# Mock IFSession
|
||||
# Mock IFSession - restore now happens in start() before thread launches
|
||||
mock_session = Mock()
|
||||
mock_session.start = AsyncMock(return_value="Welcome to Zork!")
|
||||
mock_session._do_restore = AsyncMock(
|
||||
return_value="West of House\nYou are standing in an open field."
|
||||
restored_output = (
|
||||
"restoring saved game...\r\nrestored.\r\n\r\n"
|
||||
"West of House\nYou are standing in an open field."
|
||||
)
|
||||
mock_session.save_path = Mock(spec=Path)
|
||||
mock_session.save_path.exists = Mock(return_value=True)
|
||||
mock_session.start = AsyncMock(return_value=restored_output)
|
||||
|
||||
with patch("mudlib.commands.play.IFSession") as MockIFSession:
|
||||
MockIFSession.return_value = mock_session
|
||||
|
|
@ -154,19 +151,15 @@ async def test_play_restores_save_if_exists(player):
|
|||
|
||||
await cmd_play(player, "zork1")
|
||||
|
||||
# Verify restore was called
|
||||
mock_session._do_restore.assert_called_once()
|
||||
|
||||
# Verify session was created and started
|
||||
mock_session.start.assert_called_once()
|
||||
|
||||
# Verify mode was pushed
|
||||
assert "if" in player.mode_stack
|
||||
|
||||
# Verify restored text was sent
|
||||
# Verify restored text was sent (start() returns full output with restore)
|
||||
calls = [call[0][0] for call in player.writer.write.call_args_list]
|
||||
full_output = "".join(calls)
|
||||
assert "restoring" in full_output.lower()
|
||||
assert "West of House" in full_output
|
||||
assert "open field" in full_output
|
||||
|
||||
|
|
@ -174,16 +167,12 @@ async def test_play_restores_save_if_exists(player):
|
|||
@pytest.mark.asyncio
|
||||
async def test_play_no_restore_if_no_save(player):
|
||||
"""Playing does not restore if no save file exists."""
|
||||
from pathlib import Path
|
||||
|
||||
from mudlib.commands.play import cmd_play
|
||||
|
||||
# Mock IFSession
|
||||
mock_session = Mock()
|
||||
mock_session.start = AsyncMock(return_value="Welcome to Zork!")
|
||||
mock_session._do_restore = AsyncMock(return_value="")
|
||||
mock_session.save_path = Mock(spec=Path)
|
||||
mock_session.save_path.exists = Mock(return_value=False)
|
||||
|
||||
with patch("mudlib.commands.play.IFSession") as MockIFSession:
|
||||
MockIFSession.return_value = mock_session
|
||||
|
|
@ -194,9 +183,6 @@ async def test_play_no_restore_if_no_save(player):
|
|||
|
||||
await cmd_play(player, "zork1")
|
||||
|
||||
# Verify restore was NOT called
|
||||
mock_session._do_restore.assert_not_called()
|
||||
|
||||
# Verify session was created and started
|
||||
mock_session.start.assert_called_once()
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue