Skip to content

bpo-30624 remaining bare except#2108

Merged
giampaolo merged 3 commits into
masterfrom
selectors-bare-except-2
Jun 12, 2017
Merged

bpo-30624 remaining bare except#2108
giampaolo merged 3 commits into
masterfrom
selectors-bare-except-2

Conversation

@giampaolo

Copy link
Copy Markdown
Contributor

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

@mention-bot

Copy link
Copy Markdown

@giampaolo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @cf-natali, @1st1 and @serhiy-storchaka to be potential reviewers.

@1st1

1st1 commented Jun 11, 2017

Copy link
Copy Markdown
Member

Please write a complete commit message.

Comment thread Lib/selectors.py
try:
self._selector.register(key.fd, poller_events)
except Exception:
except:

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Member

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).

Copy link
Copy Markdown
Member

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.

@1st1 1st1 Jun 12, 2017

Copy link
Copy Markdown
Member

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.

@vstinner

Copy link
Copy Markdown
Member
         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 1st1 left a comment

Copy link
Copy Markdown
Member

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.

@giampaolo
giampaolo merged commit ced36a9 into master Jun 12, 2017
@giampaolo
giampaolo deleted the selectors-bare-except-2 branch June 12, 2017 12:43
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

Successfully merging this pull request may close these issues.

7 participants