Skip to content

bpo-30624 remaining bare except #2108

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 12, 2017
Merged

bpo-30624 remaining bare except #2108

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6cf19f6
forgot to fix one bare except
giampaolo Jun 11, 2017
293759c
bpo-30624 / selectors: use bare 'except' clause instead of 'except
giampaolo Jun 11, 2017
730cc05
Merge branch 'selectors-bare-except-2' of github.com:python/cpython i…
giampaolo Jun 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Lib/selectors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ def register(self, fileobj, events, data=None):
poller_events |= self._EVENT_WRITE
try:
self._selector.register(key.fd, poller_events)
except Exception:
except:
Copy link
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
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
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
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
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.

Copy link
Member

@1st1 1st1 Jun 12, 2017 •
edited
Loading

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.

super().unregister(fileobj)
raise
return key
Expand Down