Skip to content
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.

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

Remove unecessary try/finally: in _multicall #277

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bluetech
Copy link
Member

@bluetech bluetech commented Jun 7, 2020

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

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`.
@goodboy
Copy link
Contributor

goodboy commented Jun 7, 2020

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 goodboy changed the title Remove useless try in _multicall Remove unecessary try/finally: in _multicall Feb 17, 2021
@goodboy
Copy link
Contributor

goodboy commented Feb 17, 2021

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.

@bluetech
Copy link
Member Author

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.

@goodboy
Copy link
Contributor

goodboy commented Mar 27, 2021

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
Copy link
Contributor

goodboy commented Mar 27, 2021

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.

results.append(res)
if firstresult: # halt further impl calls
break
except BaseException:
Copy link
Contributor

@goodboy goodboy Aug 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've already stated this, but just to re-iterate: IMO instead this PR should change this to Exception and we shouldn't be handling BaseException at all (unless we need it for GeneratorExit - in which case we likely need a separate block anyway).

@RonnyPfannschmidt
Copy link
Member

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

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.

3 participants