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

Use BaseException instead of Exception to catch GeneratorExit failures #919

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SylvainCorlay
Copy link
Member

Thanks to @maartenbreddels for taking the time to investigate this!

@SylvainCorlay SylvainCorlay marked this pull request as draft July 2, 2021 08:19
@SylvainCorlay
Copy link
Member Author

Nice explanation about GeneratorExit exceptions when breaking loops in generators https://www.peterbe.com/plog/generatorexit.

@jasongrout
Copy link
Member

Filling in one more detail from the docs: https://docs.python.org/3/library/exceptions.html#GeneratorExit - GeneratorExit inherits from BaseException "since it is technically not an error."

@SylvainCorlay
Copy link
Member Author

I don't think this is actually the right fix to the issue... I am investigating more.

@jasongrout
Copy link
Member

I was trying to figure out when you would get a GeneratorExit thrown in this case, but didn't get that far. Is that what is bothering you about this solution?

Since you do have a finally clause, you could also put the fallback into the finally (define output_cell above as None, and check to see if it is None in the finally and if it is, set it to the default). That's not figuring why there is an error, but it at least does make sure to define output_cell in the finally clause.

@maartenbreddels
Copy link
Member

So the error doesn't happen due to us calling someone who fails (the execute_cell code path), but someone who calls us decides to bail out early, which might be an issue in a template, since that is the one that calls _jinja_cell_generator.

@jasongrout
Copy link
Member

jasongrout commented Jul 2, 2021

That makes more sense, thanks. To summarize, did the bailing happen because of the async problem, and while it was trying to bail, that's when we got the output cell error?

@jasongrout
Copy link
Member

jasongrout commented Jul 2, 2021

A further thought: according to the docs, a generator is not allowed to yield one more time on GeneratorExit. If it does, a RuntimeError is raised: https://docs.python.org/3/reference/expressions.html#generator.close

I think that catching GeneratorExit and yielding a value is the wrong thing to do here. Perhaps output_cell can be defined as None at the top, and the finally clause can check if the output_cell is not None, and if so, yield it, and change that BaseException back to Exception.

@jasongrout
Copy link
Member

I think that catching GeneratorExit and yielding a value is the wrong thing to do here. Perhaps output_cell can be defined as None at the top, and the finally clause can check if the output_cell is not None, and if so, yield it, and change that BaseException back to Exception.

This still doesn't solve the problem with why the generator is exiting early, though?

@maartenbreddels
Copy link
Member

maartenbreddels commented Jul 2, 2021 via email

@jasongrout
Copy link
Member

No, I wonder if this improper handling hides the original exception.

In the tracebacks I've seen, it has been something like what you posted about in erdewit/nest_asyncio#22, but it says that while it was handling that error, another error happened, which is this output_cell reference error. So I don't think it's masking the original exception, thanks to Python 3 exception chaining. However, there is still something more going on, maybe related to erdewit/nest_asyncio#22

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