Skip to content

Conversation

@andrewnester
Copy link
Contributor

@nikic
Copy link
Member

nikic commented Oct 23, 2017

I believe we shouldn't be running finally blocks in generators on unclean shutdown (exit, fatal error). We also don't run finally blocks if exit used in normal (non-generator) codes. Do you agree @bwoebi @kelunik?

@bwoebi
Copy link
Member

bwoebi commented Oct 23, 2017

@nikic I agree on this. Actually, why do we even run destructors at all upon unclean shutdown?

@nikic
Copy link
Member

nikic commented Oct 23, 2017

@bwoebi We sort-of don't. Destructors are run on unclean shutdown, but for fatal errors specifically we explicitly mark all objects as destructed prior to bailout. So basically "exit" is the only (common) unclean shutdown scenario where we actually do run destructors.

@bwoebi
Copy link
Member

bwoebi commented Oct 23, 2017

Then I agree for fatal errors (I honestly don't care too much about that specific case), but generators not be special cased relative to all other destructors. The destructor of the generator is to run finally, and so shall it run when all other destructors are called.

@nikic
Copy link
Member

nikic commented Oct 23, 2017

@bwoebi The generator dtor is only there to run finally though, and we do not run finally blocks on unclean shutdown (independent on whether it's using exit or fatal error). So I'd say it's consistent to not run them here either.

Btw I think we should run finally blocks on exit in general, but that's a more general issue.

@bwoebi
Copy link
Member

bwoebi commented Oct 23, 2017

@nikic I had written #1352 some time ago, but don't really want to bother RFCing it...

@krakjoe
Copy link
Member

krakjoe commented Oct 25, 2017

I guess we are unhappy with the approach here ... any plans to implement any other solution ?

@andrewnester
Copy link
Contributor Author

I am closing this pul request at the moment as new solution need to be introduced (will try to make it in separate CR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants