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
|
@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. |
|
Please write a complete commit message. |
| @@ -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: | |||
There was a problem 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?
There was a problem 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 exThere was a problem 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.
There was a problem 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).
There was a problem 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.
There was a problem 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.
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. |
…nto selectors-bare-except-2
In #2082 I forgot to fix one bare except clause.