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

@logger.catch decorator's default behavior to suppress exceptions is not intuitive #1016

Open
MAGLeb opened this issue Nov 2, 2023 · 1 comment
Labels
enhancement Improvement to an already existing feature

Comments

@MAGLeb
Copy link

MAGLeb commented Nov 2, 2023

Problem Description:

While utilizing the @logger.catch decorator from the loguru library, I encountered behavior that seems counterintuitive. As per the current implementation, if the reraise parameter is not explicitly set, the decorator suppresses exceptions after logging them. This could lead to serious errors needing developer attention going unnoticed, as program execution continues despite the occurrence of an exception.

Proposed Solution:

I suggest changing the default behavior of the @logger.catch decorator to re-raise the exception (i.e., the reraise parameter should default to True). This will align the decorator's behavior with the expectations of developers who are accustomed to using other libraries and frameworks where exception catching decorators do not suppress exceptions unless explicitly instructed to.

Rationale:

A decorator's behavior that deviates from the majority of developers' expectations can lead to debugging difficulties and even production errors, as not all exceptions will be properly handled or noticed. The proposed change will make loguru usage safer and more intuitive.

Current Behavior Example:

from loguru import logger

@logger.catch
def function_that_raises():
    raise ValueError("Something went wrong!")

function_that_raises()
# The program continues execution; the exception is suppressed.
print("It works here!")

Proposed Behavior Example:

from loguru import logger

@logger.catch  # Assuming reraise=True by default
def function_that_raises():
    raise ValueError("Something went wrong!")

function_that_raises()
# The exception is logged and also re-raised, causing the program to halt.

Conclusion:

I am looking forward to discussing this proposal and am willing to participate in further development and testing of this change. Thank you for considering this issue and for your work on loguru!

@Delgan
Copy link
Owner

Delgan commented Nov 5, 2023

Hi @MAGLeb.

Thank you for your thoughtful consideration of logger.catch() design and the detailed arguments you provided.

I think you're right.
This method was notably intended to wrap application's main() function and thread workers to ensure that errors were captured and logged. However, the default being reraise=False causes the application to terminate with a 0 exit status, which is problematic. The current workaround is to use logger.catch(onerror=lambda _: sys.exit(1)) but this bothers me because it's a rather convoluted solution for a behavior that should be the default one.
More generally, I don't like the idea of the logger interfering with the program's control flow. That's not its role, and it may be that the logger.catch() method has gained functionality beyond its original intend.

However, the drawback of reraise=True is that it causes the traceback to be duplicated on sys.stderr:

from loguru import logger


def boom():
    1 / 0

@logger.catch(reraise=True)
def main():
    boom()

if __name__ == "__main__":
    main()
2023-11-05 16:39:27.909 | ERROR    | __main__:<module>:12 - An error has been caught in function '<module>', process 'MainProcess' (109257), thread 'MainThread' (139716154570560):
Traceback (most recent call last):

> File "/home/delgan/programming/loguru/test.py", line 12, in <module>
    main()
    └ <function main at 0x7f123239a520>

  File "/home/delgan/programming/loguru/test.py", line 9, in main
    boom()
    └ <function boom at 0x7f1232f14540>

  File "/home/delgan/programming/loguru/test.py", line 5, in boom
    1 / 0

ZeroDivisionError: division by zero
Traceback (most recent call last):
  File "/home/delgan/programming/loguru/test.py", line 12, in <module>
    main()
  File "/home/delgan/programming/loguru/loguru/_logger.py", line 1277, in catch_wrapper
    return function(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/delgan/programming/loguru/test.py", line 9, in main
    boom()
  File "/home/delgan/programming/loguru/test.py", line 5, in boom
    1 / 0
    ~~^~~
ZeroDivisionError: division by zero

This is not acceptable for the user experience, in my opinion. We need to think about a possible solution to this problem.

Going back to what I said earlier, perhaps we can imagine a new exit parameter equal to True by default. On error, it would automatically call sys.exit(1) which internally raises a SystemExit exception. This makes sure the captured error isn't silently ignored, while also preventing the traceback to be displayed twice. If exit=False, the error could be re-raised as is after being logged.

This idea goes even further than your suggestion, since it implies to remove altogether the reraise, onerror and default arguments.

In any case, if we decide to update the API, you also need to be aware of risks of introducing breaking changes. Loguru is not yet in version 1.0.0, so theoretically no guarantee regarding the API stability are given. However, it would be preferable if we could introduce this gently with depreciation warnings.

@Delgan Delgan added the enhancement Improvement to an already existing feature label Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to an already existing feature
Projects
None yet
Development

No branches or pull requests

2 participants