When a client has already closed the connection, error handlers registered with `@klein.app.Klein(...).handle_errors()` do not get invoked, and instead a traceback is always logged

[Murtagy]

When the user(client) closes the connection the defer.CancelledError may interrupt any await operation.

This makes the code in API like

try:
    do_work()
except Exception:
    raise APIError()

to cause logs such as

Unhandled Error Processing Request.
Traceback (most recent call last):
...

located here

klein/src/klein/_resource.py

Line 270 in a15e417

if not failure.check(defer.CancelledError): # pragma: no branch

I would consider calling the user-defined callbacks still on a an exception on a finished request.
Although the error handler can not or should not write any response down - some other handled behaviour might be done.

I was emulating this locally with this code snippet

import socket
from urllib.parse import urlparse

url = 'http://127.0.0.1:8000/'

def emulate_abrupt_disconnect(url: str) -> None:
    parsed_url = urlparse(url)
    host = parsed_url.hostname
    port = parsed_url.port or 80
    path = parsed_url.path + ('?' + parsed_url.query if parsed_url.query else '')

    request = (
        f"GET {path} HTTP/1.1\r\n"
        f"Host: {host}\r\n"
        "User-Agent: PythonSocketClient/1.0\r\n"
        "Connection: close\r\n"
        "\r\n"
    )
    with socket.create_connection((host, port), timeout=5) as sock:
        sock.sendall(request.encode('utf-8'))
        # Abruptly close the socket without reading the response
        sock.shutdown(socket.SHUT_RDWR)
        sock.close()

emulate_abrupt_disconnect(url)

[glyph]

I'm sorry, I don't think I understand the bug report. Are you just asking for https://docs.twistedmatrix.com/en/stable/api/twisted.web.http.Request.html#notifyFinish ? This is already on Klein's request, as it inherits from Twisted's.

[Murtagy]

I'm sorry, I don't think I understand the bug report. Are you just asking for https://docs.twistedmatrix.com/en/stable/api/twisted.web.http.Request.html#notifyFinish ? This is already on Klein's request, as it inherits from Twisted's.

Sorry for being not very clear.
The issue is in this snippet

        if request_finished[0]:
                # TODO: coverage
                if not failure.check(defer.CancelledError):  # pragma: no branch
                    log.err(
                        failure, "Unhandled Error Processing Request."
                    )  # pragma: no cover
                return None

If a request either get's cancelled or dropped - the app code is interrupted with an CancelledError.
This CancelledError may interrupt any awaitable.
e.g. in our case we do something like this

try:
    await lookup_prices()
except Exception:
    raise PriceNotFound()

And our error handler takes this PriceNotFound exception and structures it into JSON object for the client.

The upper snippet from the framework would kick in and log Unhandled Error Processing Request if await lookup_prices() would raise CancelledError. Because it would check that the request is finished, but the failure is not CancelledError.
IMO it is not realistic that we make each path aware of potential CancelledError.
So I would either suggest not log at an ERROR level or still call the user defined callbacks on this failure, although the response would not be delivered to the caller

[glyph]

@Murtagy Ah, okay. I think I see the problem: you are saying that @app.handle_errors(CancelledError) ought to work. And yeah, I think I agree, although I think that as a pattern, that would be risky, and I would advise against it.

[glyph]

To be clear about the risk here: the goal of handle_errors is to allow you to translate known error conditions like an HTTPError from your application code, one that you know is going to be raised in a few discrete places, and is going to be safe to handle without logging, and in particular without logging a traceback. If your code raises MyAppError(UNKNOWN_THING) to mean HTTP 404, that's fine.

By contrast, defer.CancelledError (and asyncio.CancelledError) are potentially raised from any await operation — not just from "request cancelled by HTTP client aborting early", but by anything that causes a call to .cancel(), and thus really ought to be handled at the call site, and show you a traceback if not.

That said, I think you are correct in that this logic also applies to MemoryError, KeyError, ValueError, StopIteration, and basically every other stdlib exception to some degree, and we do not have special cases to avoid their use. So we should fix it so that this behaves uniformly, but also think about some way to discourage it so that people don't overly-aggressively silence generalized exceptions.

The discouragement can be a separate ticket though, no need to pile that on to the same fix (as I said, the problem already exists with a bunch of other general exceptions).

[Murtagy]

@Murtagy Ah, okay. I think I see the problem: you are saying that @app.handle_errors(CancelledError) ought to work. And yeah, I think I agree, although I think that as a pattern, that would be risky, and I would advise against it.

The main frustration point for me is that app raised PriceNotFoundError error which is supposed to reach the error_handler, but it does not reach it and instead logs an error at the framework level.

So, we have caught CancelledError and re-raised with another home-written http exception (PriceNotFoundError) - it still hits the path of cancelled or finished request and writes the error log.
The log is at ERROR level due to the only expected type being CancelledError (if not failure.check(defer.CancelledError)), but it gets PriceNotFoundError and thus logs log.error(failure, "Unhandled Error Processing Request.")

Which is a problem for us because we wake on-call person if the number of error logs is above the bar.
We don't code in a way that we expect any awaitable to raise CancelledError and treat is as a special case, and in a way it is not.
So I was suggesting that this if branch deals only with CancelledError and other error types have a chance to reach user-defined error handlers

[glyph]

So, we have caught CancelledError and re-raised with another home-written http exception (PriceNotFoundError) - it still hits the path of cancelled or finished request and writes the error log.

Thanks for the clarification. This is both a bug in Klein and a totally valid / correct error handling pattern. We should definitely fix this soon.

[glyph]

So, I understand that this is creating a broken situation, but the reason that this behaves the way that it does is that we are expecting the error handlers to convert an error in application logic into a response to be handled by the client. Invoking this conversion and then discarding the result is potentially confusing, because you might i.e. return a resource that you would expect to be rendered, then not have its render method get invoked and not see any log messages.