Kludex / mangum

AWS Lambda support for ASGI applications
http://mangum.fastapiexpert.com/
MIT License
1.71k stars 127 forks source link

Allow option to let unhandled Exceptions propagate through the adapter #285

Open jb3rndt opened 1 year ago

jb3rndt commented 1 year ago

Hi, thank you for this nice tool to wrap asgi applications. It makes deploying an API to lambda very easy while using a server to emulate it locally.

However, I found out that, in case of unhandled runtime errors in my code, the adapter automatically returns a json with a status code of 500. While that is totally fine, the original error gets caught in the HTTPCycle.run method which makes a manual handling of all uncaught errors impossible (afaik). I want to let the error propagate to the lambda execution runtime, so that the Cloudwatch "ERROR" metric for the function recognizes the execution as a failure and thus the alarms are triggered. The way it is right now, all errors are just caught and not even aws knows that something unexpected happened.

Is there a way to fix that or something essential I am missing? One solution could be to optionally reraise all exceptions in the mentioned method.

Thanks 👋

jordaneremieff commented 1 year ago

@jb3rndt I recall some similar discussions in the past and that there may have been issue unrelated to the exception handling, but don't remember the details.

Could you post a minimal example that demonstrates your use-case?

jb3rndt commented 1 year ago

Hi, yes here is a minimal example: It should demonstrate that it is not possible to have a custom exception handler for all uncaught exceptions, because they are already caught and printed inside Mangum.

from fastapi import FastAPI
from mangum import Mangum

app = FastAPI()

@app.get("/")
def test_route():
    raise Exception("This is a test exception")

def handler(event, context):
    try:
        return Mangum(app)(event, context)
    except Exception as e:
        # Custom exception handling in the app root for all unhandled exceptions (not executed in Lambda)
        print(repr(e))

event = {
    "resource": "/",
    "path": "/",
    "httpMethod": "GET",
    "headers": {
        "accept": "text/html",
    },
    "queryStringParameters": None,
    "multiValueQueryStringParameters": None,
    "pathParameters": None,
    "stageVariables": None,
    "requestContext": {
        "resourceId": "2gxmpl",
        "resourcePath": "/",
        "httpMethod": "GET",
        "extendedRequestId": "JJbxmplHYosFVYQ=",
        "requestTime": "10/Mar/2020:00:03:59 +0000",
        "path": "/Prod/",
        "accountId": "123456789012",
        "protocol": "HTTP/1.1",
        "stage": "Prod",
        "domainPrefix": "70ixmpl4fl",
        "requestTimeEpoch": 1583798639428,
        "requestId": "77375676-xmpl-4b79-853a-f982474efe18",
        "domainName": "70ixmpl4fl.execute-api.us-east-2.amazonaws.com",
        "apiId": "70ixmpl4fl"
    },
    "body": None,
    "isBase64Encoded": False
}

if __name__ == "__main__":
    handler(event, None)
Rojas-Andres commented 1 year ago

Hi @jb3rndt why don't you try to use HTTPException from FastAPI , otherwise this part of the code can be changed like this


def handler(event, context):
    try:
        return Mangum(app)(event, context)
    except Exception as e:
        # Custom exception handling in the app root for all unhandled exceptions (not executed in Lambda)
        print(repr(e))

from fastapi import FastAPI, HTTPException
from mangum import Mangum
from fastapi import status

app = FastAPI()

# This is a extra validation
@app.exception_handler(RequestValidationError)
async def validation_exception_handler(request: Request, exc: RequestValidationError):
    return JSONResponse(
        status_code=response_status.HTTP_400_BAD_REQUEST,
        content=jsonable_encoder({"errors": exc.errors(), "body": exc.body}),
    )

@app.get("/")
def test_route():
    #raise Exception("This is a test exception")
    raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail={"error": str(e)})

handler = Mangum(app)

On the other hand if you need to retrieve the event and the context you can use starlette as follows.

from fastapi import FastAPI, HTTPException
from mangum import Mangum
from fastapi import status
from starlette.requests import Request

app = FastAPI()

# This is a extra validation
@app.exception_handler(RequestValidationError)
async def validation_exception_handler(request: Request, exc: RequestValidationError):
    return JSONResponse(
        status_code=response_status.HTTP_400_BAD_REQUEST,
        content=jsonable_encoder({"errors": exc.errors(), "body": exc.body}),
    )

@app.get("/")
def test_route(request: Request):
    #raise Exception("This is a test exception")
    raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail={"error": str(e)})

handler = Mangum(app)

I hope I have helped.

jb3rndt commented 1 year ago

I cant use HTTPException as the exception handler type because I want to catch all kinds of errors that occur during runtime to log them (with context information that I require) and reraise them afterwards.

So I could use your suggestion with Exception instead of HTTPException and have my logging both in the handler for Exception and HTTPException (and every other HTTPException type like RequestValidationException...? Seems a bit ugly and easy to forget to include the logging). Thats why I need a place where every Exception is thrown once so I can log it and reraise afterwards. Maybe its the wrong place to do that, so maybe you have other ideas.

Apart from that, some problems still remain:

  1. Mangum still logs the error in the HTTPCycle.run method (so if I log the error in the format I need, the error is logged twice)
  2. The logging in HTTPCycle.run prints every line separately in the cloudwatch console which clutters it up
  3. The failure during runtime is not recognized by the lambda environment as a failure which basically makes error metrics unusable

So without HTTPCycle.run reraising the error, im not sure how to solve those points.

DavidHospital commented 1 year ago

Any updates on this?

mattiamatrix commented 10 months ago

I am having the same issue, @jb3rndt did you find a possible solution?

jb3rndt commented 9 months ago

Yes, I just reraise the exception. Check out this fork for the fix: https://github.com/jb3rndt/mangum. To apply the change in your code without having my fork as a dependency, I recommend inheriting from the original Mangum Adapter and overriding the functions where needed. Here is the code which you can drop into your project and then use Magnum instead of Mangum. But be careful! This code might need adjustments when the original package changes, so keep that in mind and use with caution.

Code ```python import asyncio from contextlib import ExitStack from typing import Type from mangum import Mangum from mangum.types import * from mangum.protocols import LifespanCycle, HTTPCycle from mangum.protocols.http import HTTPCycleState class Magnum(Mangum): def __init__( self, app: ASGI, lifespan: LifespanMode = "auto", api_gateway_base_path: str = "/", custom_handlers: Optional[List[Type[LambdaHandler]]] = None, text_mime_types: Optional[List[str]] = None, exclude_headers: Optional[List[str]] = None, ) -> None: super().__init__(app=app, lifespan=lifespan, api_gateway_base_path=api_gateway_base_path, custom_handlers=custom_handlers, text_mime_types=text_mime_types) def __call__(self, event: LambdaEvent, context: LambdaContext) -> dict: handler = self.infer(event, context) with ExitStack() as stack: if self.lifespan in ("auto", "on"): lifespan_cycle = LifespanCycle(self.app, self.lifespan) stack.enter_context(lifespan_cycle) http_cycle = MagnumHTTPCycle(handler.scope, handler.body) http_response = http_cycle(self.app) return handler(http_response) assert False, "unreachable" # pragma: no cover class MagnumHTTPCycle(HTTPCycle): def __init__(self, scope: Scope, body: bytes) -> None: super().__init__(scope, body) def __call__(self, app: ASGI) -> Response: asgi_instance = self.run(app) loop = asyncio.get_event_loop() asgi_task = loop.create_task(asgi_instance) loop.run_until_complete(asgi_task) return { "status": self.status, "headers": self.headers, "body": self.body, } async def run(self, app: ASGI) -> None: try: await app(self.scope, self.receive, self.send) except BaseException as exc: if self.state is HTTPCycleState.REQUEST: await self.send( { "type": "http.response.start", "status": 500, "headers": [[b"content-type", b"text/plain; charset=utf-8"]], } ) await self.send( { "type": "http.response.body", "body": b"Internal Server Error", "more_body": False, } ) elif self.state is not HTTPCycleState.COMPLETE: self.status = 500 self.body = b"Internal Server Error" self.headers = [[b"content-type", b"text/plain; charset=utf-8"]] raise exc from None ```
ulzha commented 5 months ago

@jb3rndt this is looking appropriate! (And I also landed here because of wanting Cloudwatch errors metric to be truthful for a lambda.) If your approach was sufficiently battle tested maybe there's a PR you'd be keen to spawn from your fork?

jb3rndt commented 5 months ago

I am not sure what would be required to have this approach "battle tested" but I am happy to file a PR for that. Although I am unsure whether the maintainer(s) consider this change to be important? So before putting more work into it, it would be nice to know if this is desired :)