bpo-30624 remaining bare except

[giampaolo]

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

[mention-bot]

@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]

Please write a complete commit message.

[1st1]

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?

[1st1]

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

[giampaolo]

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.

[gvanrossum]

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

[serhiy-storchaka]

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]

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]

         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]

Please write a normal commit message.