From 31c041018b84ed7eea778402f91f50cc7d56a669 Mon Sep 17 00:00:00 2001 From: seem Date: Mon, 18 Nov 2024 20:00:33 +0200 Subject: [PATCH] fix deadlock when handling show_help_topic requests --- .../positron_ipykernel/positron_comm.py | 20 +++++- .../positron_ipykernel/tests/test_help.py | 62 ++++++++----------- 2 files changed, 45 insertions(+), 37 deletions(-) diff --git a/extensions/positron-python/python_files/positron/positron_ipykernel/positron_comm.py b/extensions/positron-python/python_files/positron/positron_ipykernel/positron_comm.py index 12dc816956b..dce917a28db 100644 --- a/extensions/positron-python/python_files/positron/positron_ipykernel/positron_comm.py +++ b/extensions/positron-python/python_files/positron/positron_ipykernel/positron_comm.py @@ -87,7 +87,7 @@ class PositronComm: def __init__(self, comm: comm.base_comm.BaseComm) -> None: self.comm = comm - self.send_lock = threading.Lock() + self.send_lock = threading.RLock() @classmethod def create(cls, target_name: str, comm_id: str) -> PositronComm: @@ -194,6 +194,24 @@ def handle_msg(raw_msg: JsonRecord) -> None: self.comm.on_msg(handle_msg) + def handle_msg(self, raw_msg: JsonRecord) -> None: + """ + Handle a raw JSON-RPC message from the frontend-side version of this comm. + + Parameters + ---------- + raw_msg + The raw JSON-RPC message. + """ + return self.comm.handle_msg(raw_msg) + + @property + def messages(self): + """ + Messages sent to the frontend-side version of this comm, when recorded for testing purposes. + """ + return getattr(self.comm, "messages") + def send_result(self, data: JsonData = None, metadata: Optional[JsonRecord] = None) -> None: """ Send a JSON-RPC result to the frontend-side version of this comm. diff --git a/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_help.py b/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_help.py index e09670b2f98..317bfbd614e 100644 --- a/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_help.py +++ b/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_help.py @@ -12,10 +12,10 @@ import pytest from positron_ipykernel.help import HelpService, help -from positron_ipykernel.help_comm import HelpBackendRequest, HelpFrontendEvent +from positron_ipykernel.help_comm import HelpBackendRequest, HelpFrontendEvent, ShowHelpKind from .conftest import DummyComm -from .utils import json_rpc_request +from .utils import json_rpc_notification, json_rpc_request, json_rpc_response TARGET_NAME = "target_name" @@ -36,18 +36,27 @@ def running_help_service(help_service: HelpService): @pytest.fixture -def help_comm(help_service: HelpService) -> DummyComm: +def help_comm(help_service: HelpService): """ Open a dummy comm for the help service. """ # Open a comm help_comm = DummyComm(TARGET_NAME) help_service.on_comm_open(help_comm, {}) + assert help_service._comm is not None, "Comm was not created" # Clear messages due to the comm_open help_comm.messages.clear() - return help_comm + return help_service._comm + + +@pytest.fixture +def mock_pydoc_thread(help_service, monkeypatch): + mock_pydoc_thread = Mock() + mock_pydoc_thread.url = "http://localhost:1234/" + monkeypatch.setattr(help_service, "_pydoc_thread", mock_pydoc_thread) + return mock_pydoc_thread def test_pydoc_server_starts_and_shuts_down(running_help_service: HelpService): @@ -81,6 +90,12 @@ def test_pydoc_server_styling(running_help_service: HelpService): assert "#ee77aa" not in html +def show_help_event(content: str, kind=ShowHelpKind.Url, focus=True): + return json_rpc_notification( + HelpFrontendEvent.ShowHelp.value, {"kind": kind, "focus": focus, "content": content} + ) + + @pytest.mark.parametrize( ("obj", "expected_path"), [ @@ -109,50 +124,25 @@ def test_pydoc_server_styling(running_help_service: HelpService): ], ) def test_show_help( - obj: Any, expected_path: str, help_service: HelpService, help_comm: DummyComm, monkeypatch + obj: Any, expected_path: str, help_service: HelpService, help_comm, mock_pydoc_thread ): """ Calling `show_help` should resolve an object to a url and send a `ShowHelp` event over the comm. """ - # Mock the pydoc server - url = "http://localhost:1234/" - mock_pydoc_thread = Mock() - mock_pydoc_thread.url = url - monkeypatch.setattr(help_service, "_pydoc_thread", mock_pydoc_thread) - help_service.show_help(obj) - [event] = help_comm.messages - data = event["data"] - assert data["method"] == HelpFrontendEvent.ShowHelp.value - - params = data["params"] - assert params["kind"] == "url" - assert params["focus"] - prefix = f"{url}get?key=" - assert params["content"].startswith(prefix) - assert params["content"][len(prefix) :] == expected_path - + assert help_comm.messages == [ + show_help_event(f"{mock_pydoc_thread.url}get?key={expected_path}") + ] -def test_handle_show_help_topic( - help_service: HelpService, help_comm: DummyComm, monkeypatch -) -> None: - # Mock the show_help method - mock_show_help = Mock() - monkeypatch.setattr(help_service, "show_help", mock_show_help) +def test_handle_show_help_topic(help_comm, mock_pydoc_thread) -> None: msg = json_rpc_request( HelpBackendRequest.ShowHelpTopic, {"topic": "logging"}, comm_id="dummy_comm_id" ) help_comm.handle_msg(msg) assert help_comm.messages == [ - { - "data": {"jsonrpc": "2.0", "result": True}, - "metadata": None, - "buffers": None, - "msg_type": "comm_msg", - } + json_rpc_response(True), + show_help_event(f"{mock_pydoc_thread.url}get?key=logging"), ] - - mock_show_help.assert_called_once_with("logging")