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

Do not mask underlying OSError in save_automaton_to_file #19

Closed
icezyclon opened this issue Nov 8, 2021 · 3 comments
Closed

Do not mask underlying OSError in save_automaton_to_file #19

icezyclon opened this issue Nov 8, 2021 · 3 comments

Comments

@icezyclon
Copy link
Contributor

Problem:
The underlying OSError in

print(f'Could not write to file {path}.{file_type} (Permission denied).'
is masked by a different error that does not (necessarily) match the actual problem and is actively misleading.

Explanation:
The new error message says that the file could not be written and give a "(Permission denied)" as reason.
This is not the only case were an OSError could be thrown and may actually mask important information from the exception message below.
This message is printed to stdout and the original exception is lost.

Example:

  1. Pydot writes to a temporary file. If this operation is not allowed or does not succeed, then the error message should reflect that and not point towards the path of the actual file (as it does now).
  2. If Pydot does not find or cannot use graphviz then it returns an OSError instead of a FileNotFoundError, which will be caught and overwritten by the given message (it turned out the PATH Python was using was not correctly configured, but Powershell could still find it)

Both errors happened to me which took quite some time to figure out what was going on as not one but two different errors were masked.

Solution:
You should never just mask previous error messages.
More information is always good.

  1. The simplest solution is to print out the original error message (and potentially the traceback) to stderr.
    This has the disadvantage of cluttering the output and may not contain all the information a user would want but it would be enough to get someone onto the right track with fixing the underlying problem.
    Note: Because writing happens in a separate thread, output may interleave with other output from the main thread which makes it even less clear what happened.

  2. Raise an exception (do not let the error pass silently).
    This has the advantage of making it clear an error happened and what happened but may not always be the preferred behavior eg. if the rest of an algorithm is stopped due to an uncaught exception.
    You could either

  • not catch the original error message (always raise)
  • catch only the specific error of not being able to write to the file and re-raising others. (may change if Pydot changes)
  • always re-raise the exception but add additional information

2.5) You could make a flag if the exception should pass silently or not (which should be the default? hard to say)

To 2):

Python support raising exceptions from a given context like so:

try:
    raise OSError("This is the original message")
except OSError as e:
    raise OSError("Could not read from file...") from e

which will read as:

Traceback (most recent call last):
  File "catch.py", line 5, in <module>
    raise OSError("This is the original message")
OSError: This is the original message

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "catch.py", line 7, in <module>
    raise OSError("Could not read from file...") from e
OSError: Could not read from file...

or (what you more likely want) to extend the existing exception like so:

try:
    raise OSError("This is the original message")
except OSError as e:
    e.args = e.args + ("Could not read from file...", )
    raise

which will read as:

Traceback (most recent call last):
  File "catch.py", line 2, in <module>
    raise OSError("This is the original message")
OSError: ('This is the original message', 'Could not read from file...')

For a longer discussion about the topic and also Python 2 conform solutions see: https://stackoverflow.com/questions/9157210/how-do-i-raise-the-same-exception-with-a-custom-message-in-python

@emuskardin
Copy link
Member

Thanks Felix,

I will solve try to solve that issue tomorrow, or in case if you already have the solution implemented, you can always open the pull request and I will merge it 👯

Ps. On the unrelated note, but just so that you do not "judge the code style" in AALpy, I know that FileHandler.py is a mess that requires a redesign

@emuskardin
Copy link
Member

Any idea why the following does not work? I get a trackback, but my message is not added to it.

 except OSError as e:
       e.args += (f'Could not write to file {path}.{file_type}. Try closing the file if opened.',)
       raise

@icezyclon
Copy link
Contributor Author

icezyclon commented Nov 9, 2021

Strange. It worked for me.
How are you printing the message?

This works for me in Python3.8:

try:
    try:
        raise OSError("This is the original message")
    except OSError as e:
        e.args += ("Could not read from file...",)
        raise
except OSError as e:
    print(e)

Results in output:
('This is the original message', 'Could not read from file...')

For full trace:

import traceback

try:
    try:
        raise OSError("This is the original message")
    except OSError as e:
        e.args += ("Could not read from file...",)
        raise
except OSError as e:
    traceback.print_exc()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants