From 199b5826fc8222ba37f235d9dbcb3ab52cef25c7 Mon Sep 17 00:00:00 2001 From: Edward Oakes Date: Fri, 1 Nov 2024 13:58:42 -0500 Subject: [PATCH] [serve] Return "Internal Server Error" instead of traceback on user exception (#48491) In the non-fastapi HTTP handling codepath, we currently format the internal stack trace and return it in the response. This is unexpected behavior compared to other HTTP servers because it leaks internal implementation details. This change matches the FastAPI behavior: returning a `500` status with "Internal Server Error." --------- Signed-off-by: Edward Oakes --- python/ray/serve/_private/replica.py | 17 +++++++------ python/ray/serve/_private/utils.py | 13 ---------- python/ray/serve/api.py | 2 +- python/ray/serve/tests/test_http_routes.py | 2 +- python/ray/serve/tests/test_logging.py | 15 +++++------ .../tests/unit/test_user_callable_wrapper.py | 25 +++++++++---------- 6 files changed, 31 insertions(+), 43 deletions(-) diff --git a/python/ray/serve/_private/replica.py b/python/ray/serve/_private/replica.py index d6ac68343354..d2fb2446dbd8 100644 --- a/python/ray/serve/_private/replica.py +++ b/python/ray/serve/_private/replica.py @@ -59,7 +59,7 @@ from ray.serve._private.metrics_utils import InMemoryMetricsStore, MetricsPusher from ray.serve._private.thirdparty.get_asgi_route_name import get_asgi_route_name from ray.serve._private.utils import get_component_file_name # noqa: F401 -from ray.serve._private.utils import parse_import_path, wrap_to_ray_error +from ray.serve._private.utils import parse_import_path from ray.serve._private.version import DeploymentVersion from ray.serve.config import AutoscalingConfig from ray.serve.deployment import Deployment @@ -414,7 +414,7 @@ def _wrap_user_method_call( task.cancel() except Exception as e: user_exception = e - logger.error(f"Request failed:\n{e}") + logger.exception("Request failed.") if ray.util.pdb._is_ray_debugger_enabled(): ray.util.pdb._post_mortem() finally: @@ -1234,15 +1234,16 @@ async def call_user_method( asgi_args=asgi_args, ) - except Exception as e: - e = wrap_to_ray_error(user_method_name, e) + except Exception: if request_metadata.is_http_request and asgi_args is not None: - result = starlette.responses.Response( - f"Unexpected error, traceback: {e}.", status_code=500 + await self._send_user_result_over_asgi( + starlette.responses.Response( + "Internal Server Error", status_code=500 + ), + asgi_args, ) - await self._send_user_result_over_asgi(result, asgi_args) - raise e from None + raise finally: if receive_task is not None and not receive_task.done(): receive_task.cancel() diff --git a/python/ray/serve/_private/utils.py b/python/ray/serve/_private/utils.py index 976a71ef4348..3b9a5f4f6644 100644 --- a/python/ray/serve/_private/utils.py +++ b/python/ray/serve/_private/utils.py @@ -7,7 +7,6 @@ import random import string import time -import traceback import uuid from abc import ABC, abstractmethod from decimal import ROUND_HALF_UP, Decimal @@ -24,7 +23,6 @@ from ray._private.worker import LOCAL_MODE, SCRIPT_MODE from ray._raylet import MessagePackSerializer from ray.actor import ActorHandle -from ray.exceptions import RayTaskError from ray.serve._private.common import ServeComponentType from ray.serve._private.constants import HTTP_PROXY_TIMEOUT, SERVE_LOGGER_NAME from ray.types import ObjectRef @@ -160,17 +158,6 @@ def ensure_serialization_context(): ray.util.serialization_addons.apply(ctx) -def wrap_to_ray_error(function_name: str, exception: Exception) -> RayTaskError: - """Utility method to wrap exceptions in user code.""" - - try: - # Raise and catch so we can access traceback.format_exc() - raise exception - except Exception as e: - traceback_str = ray._private.utils.format_error_message(traceback.format_exc()) - return ray.exceptions.RayTaskError(function_name, traceback_str, e) - - def msgpack_serialize(obj): ctx = ray._private.worker.global_worker.get_serialization_context() buffer = ctx.serialize(obj) diff --git a/python/ray/serve/api.py b/python/ray/serve/api.py index 3a46722039fb..f45ffd9bdbc2 100644 --- a/python/ray/serve/api.py +++ b/python/ray/serve/api.py @@ -193,7 +193,7 @@ def decorator(cls): if issubclass(cls, collections.abc.Callable): raise ValueError( - "Class passed to @serve.ingress may not have __call__ method." + "Classes passed to @serve.ingress may not have __call__ method." ) # Sometimes there are decorators on the methods. We want to fix diff --git a/python/ray/serve/tests/test_http_routes.py b/python/ray/serve/tests/test_http_routes.py index 4d7cf3e153a7..68e8b3acdf0e 100644 --- a/python/ray/serve/tests/test_http_routes.py +++ b/python/ray/serve/tests/test_http_routes.py @@ -220,7 +220,7 @@ def f(): serve.run(f.bind()) r = requests.get("http://localhost:8000/f") assert r.status_code == 500 - assert "ZeroDivisionError" in r.text, r.text + assert r.text == "Internal Server Error" @ray.remote(num_cpus=0) def intentional_kill(actor_handle): diff --git a/python/ray/serve/tests/test_logging.py b/python/ray/serve/tests/test_logging.py index e3cf663178a4..b5b723a6d987 100644 --- a/python/ray/serve/tests/test_logging.py +++ b/python/ray/serve/tests/test_logging.py @@ -738,20 +738,21 @@ def disable_stdout(): with open(logs_dir / log_file) as f: for line in f: structured_log = json.loads(line) - _message = structured_log["message"] - if "from_serve_logger" in _message: + message = structured_log["message"] + exc_text = structured_log.get("exc_text", "") + if "from_serve_logger" in message: from_serve_logger_check = True - elif "from_print" in _message: + elif "from_print" in message: from_print_check = True # Error was logged from replica directly. - elif "from_error" in _message: + elif "from_error" in exc_text: from_error_check = True - elif "direct_from_stdout" in _message: + elif "direct_from_stdout" in message: direct_from_stdout = True - elif "direct_from_stderr" in _message: + elif "direct_from_stderr" in message: direct_from_stderr = True - elif "this\nis\nmultiline\nlog\n" in _message: + elif "this\nis\nmultiline\nlog\n" in message: multiline_log = True assert from_serve_logger_check assert from_print_check diff --git a/python/ray/serve/tests/unit/test_user_callable_wrapper.py b/python/ray/serve/tests/unit/test_user_callable_wrapper.py index 2e14bd134e76..d43747845f55 100644 --- a/python/ray/serve/tests/unit/test_user_callable_wrapper.py +++ b/python/ray/serve/tests/unit/test_user_callable_wrapper.py @@ -12,7 +12,6 @@ from starlette.responses import PlainTextResponse from ray import serve -from ray.exceptions import RayTaskError from ray.serve._private.common import ( DeploymentID, RequestMetadata, @@ -152,7 +151,7 @@ def test_basic_class_callable(): # Call non-generator method with is_streaming. request_metadata = _make_request_metadata(is_streaming=True) - with pytest.raises(RayTaskError, match="did not return a generator."): + with pytest.raises(TypeError, match="did not return a generator."): user_callable_wrapper.call_user_method( request_metadata, tuple(), dict() ).result() @@ -176,7 +175,7 @@ def test_basic_class_callable(): ).result() == "hi-kwarg" ) - with pytest.raises(RayTaskError, match="uh-oh"): + with pytest.raises(RuntimeError, match="uh-oh"): user_callable_wrapper.call_user_method( request_metadata, tuple(), {"raise_exception": True} ).result() @@ -185,7 +184,7 @@ def test_basic_class_callable(): request_metadata = _make_request_metadata( call_method="call_async", is_streaming=True ) - with pytest.raises(RayTaskError, match="did not return a generator."): + with pytest.raises(TypeError, match="did not return a generator."): user_callable_wrapper.call_user_method( request_metadata, tuple(), dict() ).result() @@ -210,7 +209,7 @@ def test_basic_class_callable(): ).result() == "hi-kwarg" ) - with pytest.raises(RayTaskError, match="uh-oh"): + with pytest.raises(RuntimeError, match="uh-oh"): user_callable_wrapper.call_user_method( request_metadata, tuple(), {"raise_exception": True} ).result() @@ -230,7 +229,7 @@ async def append_to_list(item: Any): call_method="call_generator", is_streaming=False ) with pytest.raises( - RayTaskError, match="Method 'call_generator' returned a generator." + TypeError, match="Method 'call_generator' returned a generator." ): user_callable_wrapper.call_user_method( request_metadata, (10,), dict(), generator_result_callback=append_to_list @@ -247,7 +246,7 @@ async def append_to_list(item: Any): result_list.clear() # Call sync generator raising exception. - with pytest.raises(RayTaskError, match="uh-oh"): + with pytest.raises(RuntimeError, match="uh-oh"): user_callable_wrapper.call_user_method( request_metadata, (10,), @@ -262,7 +261,7 @@ async def append_to_list(item: Any): call_method="call_async_generator", is_streaming=False ) with pytest.raises( - RayTaskError, match="Method 'call_async_generator' returned a generator." + TypeError, match="Method 'call_async_generator' returned a generator." ): user_callable_wrapper.call_user_method( request_metadata, (10,), dict(), generator_result_callback=append_to_list @@ -279,7 +278,7 @@ async def append_to_list(item: Any): result_list.clear() # Call async generator raising exception. - with pytest.raises(RayTaskError, match="uh-oh"): + with pytest.raises(RuntimeError, match="uh-oh"): user_callable_wrapper.call_user_method( request_metadata, (10,), @@ -296,7 +295,7 @@ def test_basic_function_callable(fn: Callable): # Call non-generator function with is_streaming. request_metadata = _make_request_metadata(is_streaming=True) - with pytest.raises(RayTaskError, match="did not return a generator."): + with pytest.raises(TypeError, match="did not return a generator."): user_callable_wrapper.call_user_method( request_metadata, tuple(), dict() ).result() @@ -317,7 +316,7 @@ def test_basic_function_callable(fn: Callable): request_metadata, tuple(), {"suffix": "-kwarg"} ).result() ) == "hi-kwarg" - with pytest.raises(RayTaskError, match="uh-oh"): + with pytest.raises(RuntimeError, match="uh-oh"): user_callable_wrapper.call_user_method( request_metadata, tuple(), {"raise_exception": True} ).result() @@ -336,7 +335,7 @@ async def append_to_list(item: Any): # Call generator function without is_streaming. request_metadata = _make_request_metadata(is_streaming=False) with pytest.raises( - RayTaskError, match=f"Method '{fn.__name__}' returned a generator." + TypeError, match=f"Method '{fn.__name__}' returned a generator." ): user_callable_wrapper.call_user_method( request_metadata, (10,), dict(), generator_result_callback=append_to_list @@ -353,7 +352,7 @@ async def append_to_list(item: Any): result_list.clear() # Call generator function raising exception. - with pytest.raises(RayTaskError, match="uh-oh"): + with pytest.raises(RuntimeError, match="uh-oh"): user_callable_wrapper.call_user_method( request_metadata, (10,),