pluggy icon indicating copy to clipboard operation
pluggy copied to clipboard

Remove unecessary try/finally: in _multicall

Open bluetech opened this issue 5 years ago • 6 comments

In Python 3, every raisable thing inherits from BaseException, so the except BaseException in the code already handles every possible exit. So we can reduce the indentation and the slight overhead of try.

Diff better viewed with git diff -w (or add ?w=1 in github).

bluetech avatar Jun 07 '20 17:06 bluetech

Hmm, though I like this cleanup I wonder if we should wait until #260 and #244 are discussed more.

I'm wondering if we should be handling Exception vs. BaseException differently possibly making way for special internal errors that have to do with relaying info between wrappers?

Also I guess we might run into trouble if sys.exc_info() itself earns a bug, though I'm not sure in that case if it matters if the finally: stuff is absolutely executed?

Seems like a good idea on the surface for sure.

goodboy avatar Jun 07 '20 17:06 goodboy

Was just thinking about this change again for some odd reason and was thinking maybe we shouldn't be catching and packing BaseException at all?

The set of exceptions that derive from it is special: https://docs.python.org/3/library/exceptions.html#exception-hierarchy

BaseException
 +-- SystemExit
 +-- KeyboardInterrupt
 +-- GeneratorExit
 +-- Exception

Is there any reason to be catching anything but Exception sub-types?

I'm pretty sure catching a GeneratorExit could potentially be a bad thing if we end up moving towards the iterable hook calling implementation from #98 as well.

goodboy avatar Feb 17 '21 16:02 goodboy

Is there any reason to be catching anything but Exception sub-types?

pytest at least seems to rely on it. It has a couple of special control-flow BaseExceptions (Skipped, Failed). When I replace BaseException -> Exception in the pluggy loop and run the pytest test suite, using the pytest.mark.skip mark stops working, and some other failures.

I haven't analyzed exactly where pytest relies on it, so it might be possible to adjust it not to rely on it.

bluetech avatar Feb 20 '21 08:02 bluetech

I haven't analyzed exactly where pytest relies on it, so it might be possible to adjust it not to rely on it.

I feel like this might be somewhat ideal.

As far as I grok deriving from BaseException should be reserved for systems which are providing a runtime that is intimately tied with what the system does for lower level signal handling. I guess GeneratorExit is the anomaly case that gives the interpreter finer grained control over a signal for a coroutine/generator exit.

As an example see trio's cancellation type which sits behind the normal exception handling in python.

goodboy avatar Mar 27 '21 16:03 goodboy

Yeah the main thing that seems odd to me is catching any of these base exception types will likely have no value in result unboxing since they are all system level signals - the GeneratorExit is the special case and if we need it then we should handle it specifically.

goodboy avatar Mar 27 '21 16:03 goodboy

i'm deferring a merge of this until i understand the implications for async better, as a side-effect i might be ripping apart the loop anyway

RonnyPfannschmidt avatar Aug 16 '21 12:08 RonnyPfannschmidt