Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jb3rndt opened this issue Jan 3, 2023 · 9 comments
Open
Labels
more info needed Further information is requested

Comments

@jb3rndt
Copy link

jb3rndt commented Jan 3, 2023

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 jordaneremieff added the more info needed Further information is requested label Jan 7, 2023
@jordaneremieff
Copy link
Collaborator

jordaneremieff commented Jan 7, 2023

@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
Copy link
Author

jb3rndt commented Mar 1, 2023

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
Copy link

Rojas-Andres commented Apr 7, 2023

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
Copy link
Author

jb3rndt commented Apr 12, 2023

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
Copy link

Any updates on this?

@mattiamatrix
Copy link

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

@jb3rndt
Copy link
Author

jb3rndt commented Jan 20, 2024

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
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
Copy link

ulzha commented May 15, 2024

@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
Copy link
Author

jb3rndt commented May 18, 2024

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more info needed Further information is requested
Projects
None yet
Development

No branches or pull requests

6 participants