clean up log.error (#1109)
This commit is contained in:
@@ -16,7 +16,7 @@ This document contains critical information about working with this codebase. Fo
|
||||
- Public APIs must have docstrings
|
||||
- Functions must be focused and small
|
||||
- Follow existing patterns exactly
|
||||
- Line length: 88 chars maximum
|
||||
- Line length: 120 chars maximum
|
||||
|
||||
3. Testing Requirements
|
||||
- Framework: `uv run --frozen pytest`
|
||||
@@ -116,3 +116,15 @@ This document contains critical information about working with this codebase. Fo
|
||||
- Follow existing patterns
|
||||
- Document public APIs
|
||||
- Test thoroughly
|
||||
|
||||
## Exception Handling
|
||||
|
||||
- **Always use `logger.exception()` instead of `logger.error()` when catching exceptions**
|
||||
- Don't include the exception in the message: `logger.exception("Failed")` not `logger.exception(f"Failed: {e}")`
|
||||
- **Catch specific exceptions** where possible:
|
||||
- File ops: `except (OSError, PermissionError):`
|
||||
- JSON: `except json.JSONDecodeError:`
|
||||
- Network: `except (ConnectionError, TimeoutError):`
|
||||
- **Only catch `Exception` for**:
|
||||
- Top-level handlers that must not crash
|
||||
- Cleanup blocks (log at debug level)
|
||||
|
||||
@@ -160,9 +160,8 @@ def main(port: int, auth_server: str, transport: Literal["sse", "streamable-http
|
||||
mcp_server.run(transport=transport)
|
||||
logger.info("Server stopped")
|
||||
return 0
|
||||
except Exception as e:
|
||||
logger.error(f"Server error: {e}")
|
||||
logger.exception("Exception details:")
|
||||
except Exception:
|
||||
logger.exception("Server error")
|
||||
return 1
|
||||
|
||||
|
||||
|
||||
@@ -75,11 +75,10 @@ def update_claude_config(
|
||||
if not config_file.exists():
|
||||
try:
|
||||
config_file.write_text("{}")
|
||||
except Exception as e:
|
||||
logger.error(
|
||||
except Exception:
|
||||
logger.exception(
|
||||
"Failed to create Claude config file",
|
||||
extra={
|
||||
"error": str(e),
|
||||
"config_file": str(config_file),
|
||||
},
|
||||
)
|
||||
@@ -139,11 +138,10 @@ def update_claude_config(
|
||||
extra={"config_file": str(config_file)},
|
||||
)
|
||||
return True
|
||||
except Exception as e:
|
||||
logger.error(
|
||||
except Exception:
|
||||
logger.exception(
|
||||
"Failed to update Claude config",
|
||||
extra={
|
||||
"error": str(e),
|
||||
"config_file": str(config_file),
|
||||
},
|
||||
)
|
||||
|
||||
+5
-6
@@ -349,12 +349,11 @@ def run(
|
||||
|
||||
server.run(**kwargs)
|
||||
|
||||
except Exception as e:
|
||||
logger.error(
|
||||
f"Failed to run server: {e}",
|
||||
except Exception:
|
||||
logger.exception(
|
||||
"Failed to run server",
|
||||
extra={
|
||||
"file": str(file),
|
||||
"error": str(e),
|
||||
},
|
||||
)
|
||||
sys.exit(1)
|
||||
@@ -464,8 +463,8 @@ def install(
|
||||
if dotenv:
|
||||
try:
|
||||
env_dict |= {k: v for k, v in dotenv.dotenv_values(env_file).items() if v is not None}
|
||||
except Exception as e:
|
||||
logger.error(f"Failed to load .env file: {e}")
|
||||
except (OSError, ValueError):
|
||||
logger.exception("Failed to load .env file")
|
||||
sys.exit(1)
|
||||
else:
|
||||
logger.error("python-dotenv is not installed. Cannot load .env file.")
|
||||
|
||||
@@ -464,8 +464,8 @@ class OAuthClientProvider(httpx.Auth):
|
||||
await self.context.storage.set_tokens(token_response)
|
||||
|
||||
return True
|
||||
except ValidationError as e:
|
||||
logger.error(f"Invalid refresh response: {e}")
|
||||
except ValidationError:
|
||||
logger.exception("Invalid refresh response")
|
||||
self.context.clear_tokens()
|
||||
return False
|
||||
|
||||
@@ -522,8 +522,8 @@ class OAuthClientProvider(httpx.Auth):
|
||||
token_request = await self._exchange_token(auth_code, code_verifier)
|
||||
token_response = yield token_request
|
||||
await self._handle_token_response(token_response)
|
||||
except Exception as e:
|
||||
logger.error(f"OAuth flow error: {e}")
|
||||
except Exception:
|
||||
logger.exception("OAuth flow error")
|
||||
raise
|
||||
|
||||
# Add authorization header and make request
|
||||
|
||||
@@ -97,7 +97,7 @@ async def sse_client(
|
||||
)
|
||||
logger.debug(f"Received server message: {message}")
|
||||
except Exception as exc:
|
||||
logger.error(f"Error parsing server message: {exc}")
|
||||
logger.exception("Error parsing server message")
|
||||
await read_stream_writer.send(exc)
|
||||
continue
|
||||
|
||||
@@ -106,7 +106,7 @@ async def sse_client(
|
||||
case _:
|
||||
logger.warning(f"Unknown SSE event: {sse.event}")
|
||||
except Exception as exc:
|
||||
logger.error(f"Error in sse_reader: {exc}")
|
||||
logger.exception("Error in sse_reader")
|
||||
await read_stream_writer.send(exc)
|
||||
finally:
|
||||
await read_stream_writer.aclose()
|
||||
@@ -126,8 +126,8 @@ async def sse_client(
|
||||
)
|
||||
response.raise_for_status()
|
||||
logger.debug(f"Client message sent successfully: {response.status_code}")
|
||||
except Exception as exc:
|
||||
logger.error(f"Error in post_writer: {exc}")
|
||||
except Exception:
|
||||
logger.exception("Error in post_writer")
|
||||
finally:
|
||||
await write_stream.aclose()
|
||||
|
||||
|
||||
@@ -308,7 +308,7 @@ class StreamableHTTPTransport:
|
||||
session_message = SessionMessage(message)
|
||||
await read_stream_writer.send(session_message)
|
||||
except Exception as exc:
|
||||
logger.error(f"Error parsing JSON response: {exc}")
|
||||
logger.exception("Error parsing JSON response")
|
||||
await read_stream_writer.send(exc)
|
||||
|
||||
async def _handle_sse_response(
|
||||
@@ -410,8 +410,8 @@ class StreamableHTTPTransport:
|
||||
else:
|
||||
await handle_request_async()
|
||||
|
||||
except Exception as exc:
|
||||
logger.error(f"Error in post_writer: {exc}")
|
||||
except Exception:
|
||||
logger.exception("Error in post_writer")
|
||||
finally:
|
||||
await read_stream_writer.aclose()
|
||||
await write_stream.aclose()
|
||||
|
||||
@@ -52,9 +52,9 @@ async def terminate_posix_process_tree(process: Process, timeout_seconds: float
|
||||
process.terminate()
|
||||
with anyio.fail_after(timeout_seconds):
|
||||
await process.wait()
|
||||
except Exception as term_error:
|
||||
logger.warning(f"Process termination failed for PID {pid}: {term_error}, attempting force kill")
|
||||
except Exception:
|
||||
logger.warning(f"Process termination failed for PID {pid}, attempting force kill")
|
||||
try:
|
||||
process.kill()
|
||||
except Exception as kill_error:
|
||||
logger.error(f"Failed to kill process {pid}: {kill_error}")
|
||||
except Exception:
|
||||
logger.exception(f"Failed to kill process {pid}")
|
||||
|
||||
@@ -311,7 +311,7 @@ class FastMCP:
|
||||
content = await resource.read()
|
||||
return [ReadResourceContents(content=content, mime_type=resource.mime_type)]
|
||||
except Exception as e:
|
||||
logger.error(f"Error reading resource {uri}: {e}")
|
||||
logger.exception(f"Error reading resource {uri}")
|
||||
raise ResourceError(str(e))
|
||||
|
||||
def add_tool(
|
||||
@@ -961,7 +961,7 @@ class FastMCP:
|
||||
|
||||
return GetPromptResult(messages=pydantic_core.to_jsonable_python(messages))
|
||||
except Exception as e:
|
||||
logger.error(f"Error getting prompt {name}: {e}")
|
||||
logger.exception(f"Error getting prompt {name}")
|
||||
raise ValueError(str(e))
|
||||
|
||||
|
||||
|
||||
@@ -105,8 +105,8 @@ class SseServerTransport:
|
||||
# Validate that endpoint is a relative path and not a full URL
|
||||
if "://" in endpoint or endpoint.startswith("//") or "?" in endpoint or "#" in endpoint:
|
||||
raise ValueError(
|
||||
f"Given endpoint: {endpoint} is not a relative path (e.g., '/messages/'), \
|
||||
expecting a relative path(e.g., '/messages/')."
|
||||
f"Given endpoint: {endpoint} is not a relative path (e.g., '/messages/'), "
|
||||
"expecting a relative path (e.g., '/messages/')."
|
||||
)
|
||||
|
||||
# Ensure endpoint starts with a forward slash
|
||||
@@ -234,7 +234,7 @@ class SseServerTransport:
|
||||
message = types.JSONRPCMessage.model_validate_json(body)
|
||||
logger.debug(f"Validated client message: {message}")
|
||||
except ValidationError as err:
|
||||
logger.error(f"Failed to parse message: {err}")
|
||||
logger.exception("Failed to parse message")
|
||||
response = Response("Could not parse message", status_code=400)
|
||||
await response(scope, receive, send)
|
||||
await writer.send(err)
|
||||
|
||||
@@ -248,8 +248,9 @@ class StreamableHTTPServerTransport:
|
||||
# Close the request stream
|
||||
await self._request_streams[request_id][0].aclose()
|
||||
await self._request_streams[request_id][1].aclose()
|
||||
except Exception as e:
|
||||
logger.debug(f"Error closing memory streams: {e}")
|
||||
except Exception:
|
||||
# During cleanup, we catch all exceptions since streams might be in various states
|
||||
logger.debug("Error closing memory streams - may already be closed")
|
||||
finally:
|
||||
# Remove the request stream from the mapping
|
||||
self._request_streams.pop(request_id, None)
|
||||
@@ -421,10 +422,10 @@ class StreamableHTTPServerTransport:
|
||||
HTTPStatus.INTERNAL_SERVER_ERROR,
|
||||
)
|
||||
await response(scope, receive, send)
|
||||
except Exception as e:
|
||||
logger.exception(f"Error processing JSON response: {e}")
|
||||
except Exception:
|
||||
logger.exception("Error processing JSON response")
|
||||
response = self._create_error_response(
|
||||
f"Error processing request: {str(e)}",
|
||||
"Error processing request",
|
||||
HTTPStatus.INTERNAL_SERVER_ERROR,
|
||||
INTERNAL_ERROR,
|
||||
)
|
||||
@@ -451,8 +452,8 @@ class StreamableHTTPServerTransport:
|
||||
JSONRPCResponse | JSONRPCError,
|
||||
):
|
||||
break
|
||||
except Exception as e:
|
||||
logger.exception(f"Error in SSE writer: {e}")
|
||||
except Exception:
|
||||
logger.exception("Error in SSE writer")
|
||||
finally:
|
||||
logger.debug("Closing SSE writer")
|
||||
await self._clean_up_memory_streams(request_id)
|
||||
@@ -569,8 +570,8 @@ class StreamableHTTPServerTransport:
|
||||
# Send the message via SSE
|
||||
event_data = self._create_event_data(event_message)
|
||||
await sse_stream_writer.send(event_data)
|
||||
except Exception as e:
|
||||
logger.exception(f"Error in standalone SSE writer: {e}")
|
||||
except Exception:
|
||||
logger.exception("Error in standalone SSE writer")
|
||||
finally:
|
||||
logger.debug("Closing standalone SSE writer")
|
||||
await self._clean_up_memory_streams(GET_STREAM_KEY)
|
||||
@@ -585,8 +586,8 @@ class StreamableHTTPServerTransport:
|
||||
try:
|
||||
# This will send headers immediately and establish the SSE connection
|
||||
await response(request.scope, request.receive, send)
|
||||
except Exception as e:
|
||||
logger.exception(f"Error in standalone SSE response: {e}")
|
||||
except Exception:
|
||||
logger.exception("Error in standalone SSE response")
|
||||
await sse_stream_writer.aclose()
|
||||
await sse_stream_reader.aclose()
|
||||
await self._clean_up_memory_streams(GET_STREAM_KEY)
|
||||
@@ -628,10 +629,7 @@ class StreamableHTTPServerTransport:
|
||||
|
||||
# Close all request streams asynchronously
|
||||
for key in request_stream_keys:
|
||||
try:
|
||||
await self._clean_up_memory_streams(key)
|
||||
except Exception as e:
|
||||
logger.debug(f"Error closing stream {key} during termination: {e}")
|
||||
await self._clean_up_memory_streams(key)
|
||||
|
||||
# Clear the request streams dictionary immediately
|
||||
self._request_streams.clear()
|
||||
@@ -645,6 +643,7 @@ class StreamableHTTPServerTransport:
|
||||
if self._write_stream is not None:
|
||||
await self._write_stream.aclose()
|
||||
except Exception as e:
|
||||
# During cleanup, we catch all exceptions since streams might be in various states
|
||||
logger.debug(f"Error closing streams: {e}")
|
||||
|
||||
async def _handle_unsupported_request(self, request: Request, send: Send) -> None:
|
||||
@@ -765,8 +764,8 @@ class StreamableHTTPServerTransport:
|
||||
event_data = self._create_event_data(event_message)
|
||||
|
||||
await sse_stream_writer.send(event_data)
|
||||
except Exception as e:
|
||||
logger.exception(f"Error in replay sender: {e}")
|
||||
except Exception:
|
||||
logger.exception("Error in replay sender")
|
||||
|
||||
# Create and start EventSourceResponse
|
||||
response = EventSourceResponse(
|
||||
@@ -777,16 +776,16 @@ class StreamableHTTPServerTransport:
|
||||
|
||||
try:
|
||||
await response(request.scope, request.receive, send)
|
||||
except Exception as e:
|
||||
logger.exception(f"Error in replay response: {e}")
|
||||
except Exception:
|
||||
logger.exception("Error in replay response")
|
||||
finally:
|
||||
await sse_stream_writer.aclose()
|
||||
await sse_stream_reader.aclose()
|
||||
|
||||
except Exception as e:
|
||||
logger.exception(f"Error replaying events: {e}")
|
||||
except Exception:
|
||||
logger.exception("Error replaying events")
|
||||
response = self._create_error_response(
|
||||
f"Error replaying events: {str(e)}",
|
||||
"Error replaying events",
|
||||
HTTPStatus.INTERNAL_SERVER_ERROR,
|
||||
INTERNAL_ERROR,
|
||||
)
|
||||
@@ -874,8 +873,8 @@ class StreamableHTTPServerTransport:
|
||||
for message. Still processing message as the client
|
||||
might reconnect and replay."""
|
||||
)
|
||||
except Exception as e:
|
||||
logger.exception(f"Error in message router: {e}")
|
||||
except Exception:
|
||||
logger.exception("Error in message router")
|
||||
|
||||
# Start the message router
|
||||
tg.start_soon(message_router)
|
||||
@@ -885,11 +884,7 @@ class StreamableHTTPServerTransport:
|
||||
yield read_stream, write_stream
|
||||
finally:
|
||||
for stream_id in list(self._request_streams.keys()):
|
||||
try:
|
||||
await self._clean_up_memory_streams(stream_id)
|
||||
except Exception as e:
|
||||
logger.debug(f"Error closing request stream: {e}")
|
||||
pass
|
||||
await self._clean_up_memory_streams(stream_id)
|
||||
self._request_streams.clear()
|
||||
|
||||
# Clean up the read and write streams
|
||||
@@ -899,4 +894,5 @@ class StreamableHTTPServerTransport:
|
||||
await write_stream_reader.aclose()
|
||||
await write_stream.aclose()
|
||||
except Exception as e:
|
||||
# During cleanup, we catch all exceptions since streams might be in various states
|
||||
logger.debug(f"Error closing streams: {e}")
|
||||
|
||||
Reference in New Issue
Block a user