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

🎨 web-server: exception handling framework #6655

Merged

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Nov 1, 2024

What do these changes do?

This PR introduces an exception handling framework for the web-server service.

  • The interface for this framework is exposed in simcore_service_webserver.exception_handling.
  • It follows up with the idea of exception handlers maps _TO_HTTP_ERROR_MAP already used in some plugins of the webserver, e.g. in _folders

These are the highlights:

  1. Exception Handling Concept
    Inspired by Starlette's exception handlers, this implementation adapts the concept to aiohttp. Exception handlers are callables that process exceptions and determine the appropriate response to return when errors or handled exceptions occur.

The following demonstrates how exception handlers (exc_handler) integrate into the HTTP request-response lifecycle:

async def middleware_handler(request: Request, handler: Handler):
    try:      
        resp = await handler(request)   

    except exc_type as exc_value:
        resp = await exc_handler(request, exc_value)  

    finally:
        return resp
  1. Map Exception-HttpInfo
    The PR includes tools to simplify the creation of exception handlers using mappings from exceptions to HTTP info that include a status code and a message. See simcore_service_webserver.{plugin}._exception_handlers._TO_HTTP_ERROR_MAP for details.

  2. Multiple Integration Options
    Provides flexibility by supporting exception handling via:

    • Context Manager: e.g. within the body of the route's handler function
    • Decorator: e.g. a route or a groupe of routes
    • Middleware: e.g. all routes in the app

Related issue/s

  • Error handling

How to test

cd services/web/server
make install-dev
pytest -vv tests/unit/isolated/test_exception_handling*.py

Dev-ops

None

@pcrespov pcrespov self-assigned this Nov 1, 2024
Copy link

codecov bot commented Nov 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.09%. Comparing base (42476f3) to head (4705c81).
Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (42476f3) and HEAD (4705c81). Click for more details.

HEAD has 27 uploads less than BASE
Flag BASE (42476f3) HEAD (4705c81)
unittests 31 4
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6655      +/-   ##
==========================================
- Coverage   88.04%   82.09%   -5.95%     
==========================================
  Files        1550      625     -925     
  Lines       61769    31104   -30665     
  Branches     2110      263    -1847     
==========================================
- Hits        54382    25536   -28846     
+ Misses       7056     5508    -1548     
+ Partials      331       60     -271     
Flag Coverage Δ
integrationtests 64.70% <63.20%> (+11.35%) ⬆️
unittests 83.63% <100.00%> (-2.68%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 77.36% <ø> (-8.00%) ⬇️
agent ∅ <ø> (∅)
api_server ∅ <ø> (∅)
autoscaling ∅ <ø> (∅)
catalog ∅ <ø> (∅)
clusters_keeper ∅ <ø> (∅)
dask_sidecar ∅ <ø> (∅)
datcore_adapter ∅ <ø> (∅)
director ∅ <ø> (∅)
director_v2 76.53% <ø> (-14.61%) ⬇️
dynamic_scheduler ∅ <ø> (∅)
dynamic_sidecar 59.80% <ø> (-29.95%) ⬇️
efs_guardian ∅ <ø> (∅)
invitations ∅ <ø> (∅)
osparc_gateway_server 79.79% <ø> (-5.70%) ⬇️
payments ∅ <ø> (∅)
resource_usage_tracker ∅ <ø> (∅)
storage ∅ <ø> (∅)
webclient ∅ <ø> (∅)
webserver 88.50% <100.00%> (+0.99%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42476f3...4705c81. Read the comment docs.

@pcrespov pcrespov added this to the Event Horizon milestone Nov 4, 2024
@pcrespov pcrespov force-pushed the mai/aihttp-exception-handlers branch 4 times, most recently from f8e744c to f819ffc Compare November 27, 2024 14:15
@pcrespov pcrespov changed the title WIP: Mai/aihttp exception handlers 🎨 web-server: exception handling framework Nov 28, 2024
@pcrespov pcrespov marked this pull request as ready for review November 28, 2024 15:22
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice. please have a look at my comments. thanks

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

@pcrespov pcrespov force-pushed the mai/aihttp-exception-handlers branch from 391bc19 to c275789 Compare December 1, 2024 20:25
@pcrespov pcrespov enabled auto-merge (squash) December 1, 2024 20:59
Copy link

sonarqubecloud bot commented Dec 1, 2024

@pcrespov pcrespov merged commit 3dded08 into ITISFoundation:master Dec 1, 2024
83 of 90 checks passed
@pcrespov pcrespov deleted the mai/aihttp-exception-handlers branch December 2, 2024 08:41
Copy link
Contributor

@bisgaard-itis bisgaard-itis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! This is definitely a great improvement and a step in the right direction.

) -> list[type[Exception]]:
"""
Keyword Arguments:
concrete_first -- If True, concrete subclasses precede their superclass (default: {True}).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have named this subclasses_first. It is not obvious to me what concrete_first does

None,
)
):
exc_handler = self._exc_handlers_map[base_exc_type]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I am late to the party (sorry). But I am wondering if you profit from the sorting and traversing the exception types 🤔.
Python has a builtin way to resolve inherritance and you could use that to only traverse the super-types of an exception

exc_handler = None
for type_ in exc_type.__mro__:
    if exc_handler := self._exc_handlers_map.get(type_):
        break
return exc_handler

async def middleware_handler(request: web.Request, handler: WebHandler):
return await _handle_excs(handler)(request)

return middleware_handler
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Starlette/FastAPI approach to exception handlers (using this type of middleware) is quite elegant, but what always confuses me is that when I raise somewhere I don't see if and where that exception will be handled. For that I have to manually "follow" the exception up until I see it reaches the middleware. Perhaps it would make sense to have an easy way to test that a handler for a specific exception type is already handled. And maybe even ensure you cannot add two handlers for the same exception type. But I guess this is closely related to the general question of the python exception mechanism: I cannot know what a given function will raise unless I look into the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:webserver issue related to the webserver service t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants