Skip to content
Open
Changes from 2 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
62 changes: 34 additions & 28 deletions python/semantic_kernel/connectors/mcp.py
Original file line number Diff line number Diff line change
Expand Up @@ -269,15 +269,15 @@ async def __aexit__(

async def connect(self) -> None:
"""Connect to the MCP server."""
ready_event = asyncio.Event()
loop = asyncio.get_running_loop()
ready_future: asyncio.Future[None] = loop.create_future()

try:
self._current_task = asyncio.create_task(self._inner_connect(ready_event))
await ready_event.wait()
self._current_task = asyncio.create_task(self._inner_connect(ready_future))
await ready_future
except KernelPluginInvalidConfigurationError:
ready_event.clear()
raise
except Exception as ex:
ready_event.clear()
await self.close()
raise FunctionExecutionException("Failed to enter context manager.") from ex

Expand All @@ -293,16 +293,20 @@ async def close(self) -> None:
self._current_task = None
self.session = None

async def _inner_connect(self, ready_event: asyncio.Event) -> None:
async def _inner_connect(self, ready_future: asyncio.Future) -> None:
if not self.session:
try:
transport = await self._exit_stack.enter_async_context(self.get_mcp_client())
except Exception as ex:
await self._exit_stack.aclose()
ready_event.set()
raise KernelPluginInvalidConfigurationError(
"Failed to connect to the MCP server. Please check your configuration."
) from ex
if not ready_future.done():
exc = KernelPluginInvalidConfigurationError(
"Failed to connect to the MCP server. Please check your configuration."
)
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.

Unlike raise KernelPluginInvalidConfigurationError(...) from ex, manually setting __cause__ does not set __suppress_context__ = True. This means Python will print 'During handling of the above exception…' instead of 'The above exception was the direct cause of…'. Add exc.__suppress_context__ = True after setting __cause__.

Suggested change
)
exc.__cause__ = ex
exc.__suppress_context__ = True
ready_future.set_exception(exc)

exc.__cause__ = ex
ready_future.set_exception(exc)
return

try:
session = await self._exit_stack.enter_async_context(
ClientSession(
Expand All @@ -316,16 +320,24 @@ async def _inner_connect(self, ready_event: asyncio.Event) -> None:
)
except Exception as ex:
await self._exit_stack.aclose()
raise KernelPluginInvalidConfigurationError(
"Failed to create a session. Please check your configuration."
) from ex
if not ready_future.done():
exc = KernelPluginInvalidConfigurationError(
"Failed to create a session. Please check your configuration."
)
exc.__cause__ = ex
ready_future.set_exception(exc)
return
try:
await session.initialize()
except Exception as ex:
await self._exit_stack.aclose()
raise KernelPluginInvalidConfigurationError(
"Failed to initialize session. Please check your configuration."
) from ex
if not ready_future.done():
exc = KernelPluginInvalidConfigurationError(
"Failed to initialize session. Please check your configuration."
)
exc.__cause__ = ex
ready_future.set_exception(exc)
return
self.session = session
elif self.session._request_id == 0:
# If the session is not initialized, we need to reinitialize it
Expand All @@ -344,7 +356,8 @@ async def _inner_connect(self, ready_event: asyncio.Event) -> None:
except Exception:
logger.warning("Failed to set log level to %s", logger.level)
# Setting up is complete, will now signal the main loop that we are ready
Comment on lines 334 to 358
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.

Deadlock bug: If load_tools(), load_prompts(), session.initialize() (line 332), or set_logging_level() raises an unhandled exception, the task exits but ready_future is never resolved—connect() hangs forever on await ready_future. Wrap this entire block in a try/except that resolves the future on failure, consistent with the earlier error paths in _inner_connect.

Suggested change
try:
if self.load_tools_flag:
await self.load_tools()
if self.load_prompts_flag:
await self.load_prompts()
if logger.level != logging.NOTSET:
try:
await self.session.set_logging_level(
next(level for level, value in LOG_LEVEL_MAPPING.items() if value == logger.level)
)
except Exception:
logger.warning("Failed to set log level to %s", logger.level)
# Setting up is complete, will now signal the main loop that we are ready
if not ready_future.done():
ready_future.set_result(None)
except Exception as ex:
await self._exit_stack.aclose()
if not ready_future.done():
ready_future.set_exception(ex)
return

ready_event.set()
if not ready_future.done():
ready_future.set_result(None)
# Create a stop event to signal the exit stack to close
self._stop_event = asyncio.Event()
await self._stop_event.wait()
Expand Down Expand Up @@ -466,11 +479,8 @@ async def message_handler(

async def load_prompts(self):
"""Load prompts from the MCP server."""
try:
prompt_list = await self.session.list_prompts()
except Exception:
prompt_list = None
for prompt in prompt_list.prompts if prompt_list else []:
prompt_list = await self.session.list_prompts()
for prompt in prompt_list.prompts:
local_name = _normalize_mcp_name(prompt.name)
func = kernel_function(name=local_name, description=prompt.description)(
partial(self.get_prompt, prompt.name)
Expand All @@ -480,12 +490,8 @@ async def load_prompts(self):

async def load_tools(self):
"""Load tools from the MCP server."""
try:
tool_list = await self.session.list_tools()
except Exception:
tool_list = None
# Create methods with the kernel_function decorator for each tool
for tool in tool_list.tools if tool_list else []:
tool_list = await self.session.list_tools()
for tool in tool_list.tools:
local_name = _normalize_mcp_name(tool.name)
func = kernel_function(name=local_name, description=tool.description)(partial(self.call_tool, tool.name))
func.__kernel_function_parameters__ = _get_parameter_dicts_from_mcp_tool(tool)
Expand Down