bpo-30624 remaining bare except by giampaolo · Pull Request #2108 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation9 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

In #2082 I forgot to fix one bare except clause.

Please write a complete commit message.

Choose a reason for hiding this comment

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

So you are changing the behaviour here -- now you will intercept BaseExceptions. Is that intended? What if a SystemExit occurs and you override it with some exception that unregister might rise?

Choose a reason for hiding this comment

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

I just checked the #2082 pr. FWIW I think that the correct code would be this:

     try:
          self._selector.register(key.fd, poller_events)
     except Exception:
          super().unregister(fileobj)
          raise
     except BaseException as ex:
          try:
              super().unregister(fileobj)
          finally:
              raise ex

Choose a reason for hiding this comment

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

I'm not sure I understand. Exceptions on Python 3 are chained, so in case of error on unregister the previous exception will still be shown. As such, there's no need of the try/finally.

Choose a reason for hiding this comment

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

I'm guessing it matters if the original exception was SystemExit, unregister were to raise e.g. KeyError, and there was a KeyError exception handler active -- even though exceptions are chained, without the finally it would still raise an instance of KeyError which would be caught, rather than raising SystemExit which would not be caught. Then again such a KeyError handler would have to be considered overly broad (if it could catch exceptions from a selector.register() call).

Choose a reason for hiding this comment

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

This is very uncommon code. If it is needed we have much more general problem. Every except ... raise should be rewritten in similar way. Maybe even every finally block. This is too cumbersome and may even need syntax support or changing the semantic.

Choose a reason for hiding this comment

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

This is very uncommon code. If it is needed we have much more general problem.

In general it is needed when you catch BaseExceptions. That's why you don't want to catch them at all usually.

For this particular code, I think @Haypo is right. I looked through the unregister method implementations and it looks like they don't raise exceptions (or can have unexpected ones). So probably a bare except/raise should work here.

         except BaseException as ex:
              try:
                  super().unregister(fileobj)
              finally:
                  raise ex

IHMO it's overkill. _BaseSelectorImpl.unregister() catchs and ignores KeyError. Calls to the select module can raise OSError exceptions, but these exceptions are ignored as well.

If you see code in unregister() that can raise a different exceptions, I would prefer to fix unregister() than writing complex code when calling unregister.

1st1 approved these changes Jun 12, 2017

Member

1st1 left a comment

Choose a reason for hiding this comment

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

Please write a normal commit message.

…nto selectors-bare-except-2