-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix GH-10026: Garbage collection stops after exception in SoapClient #10283
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
Conversation
This allows users to bail out of a try-catch without protecting the GC in case the error is recoverable and normal execution will continue.
Co-authored-by: abienvenu <abi@abienvenu.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds sensible to me. TBH I don't know why the gc_protect() is necessary. I'm not sure if it's just there to improve performance on fatal shutdowns or to protect from some kind of memory corruption, tests pass without it.
@dstogov Does this look ok to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could pick a more generic name, like _zend_nonfatal_bailout to make it more clear which is to be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion about this PR yet.
Another possible approach is to remember and restore the original value of gc_protected() in the "safe" catch places.
Note that zend_bailuot() also sets CG(unclean_shutdown) that may prevent automatic GC and "safe" destruction of global symbol table. We may also keep it in "safe" places.
BTW I can't be sure when zend_bailout() and zend_catch are safe. It's possible to prove safety only if we know both source and destination of longjmp().
I don't like zend_bailout_without_gc_protect() name. I would prefer zend_safe/clean_bailout or zend_bailout_safe/clean.
@dstogov Can you explain why gc_protect(1); is there in the first place? Is it to avoid unnecessary additions to the GC buffer that will never be cleaned up? We can document this in a comment. This will make it easier to understand whether using the new method that doesn't protect the GC is safe to use.
Note that gc_bailout() wasn't designed to be caught and continue normal execution
- CG(unclean_shutdown) - disabled GC run at shut-down
- gc_protect() - disabled future GC root collection
Note that [zend]_bailout() wasn't designed to be caught and continue normal execution
We could try to stop doing this in master.
From my understanding, SOAP leverages zend_catch to convert fatal errors into exceptions. We could do it the other way around.
Unfortunately, SOAP does that for all errors, not only SOAP ones, and it will not be possible to stop using zend_catch for these. Maybe we should stop catching these, as this will result in crashes or memory leaks. Also, fatal errors are less frequent now that many have been replaced by exceptions.
Note that [zend]_bailout() wasn't designed to be caught and continue normal execution
We could try to stop doing this in master.
From my understanding, SOAP leverages zend_catch to convert fatal errors into exceptions. We could do it the other way around.
It would be great to have the other way. I didn't have a better idea.
Unfortunately, SOAP does that for all errors, not only SOAP ones
This is done on purpose. SOAP server talks to SOAP clients and in case of PHP fatal errors, it should respond in a way that might be understand by clients.
, and it will not be possible to stop using zend_catch for these. Maybe we should stop catching these, as this will result in crashes or memory leaks. Also, fatal errors are less frequent now that many have been replaced by exceptions.
Fatal errors in SOAP client may be replaced by exceptions, but this will require manual error recovery along the C call stack (releasing all the allocated data).
This is done on purpose. SOAP server talks to SOAP clients and in case of PHP fatal errors, it should respond in a way that might be understand by clients.
I see, this didn't occur to me. This appears to be done in a safe way in SOAPServer, though, as this always bailouts in the end. soap_error_handler responds to the client, and then bailouts.
Fatal errors in SOAP client may be replaced by exceptions, but this will require manual error recovery along the C call stack (releasing all the allocated data).
👍
Closes GH-10026
In some places where zend_bailout() is used, PHP recovers and normal execution of the script can continue. This is problematic because zend_bailout() protects the GC; causing garbage not to be collected in the future. This PR introduces a new variant of zend_bailout(): zend_bailout_without_gc_protect(). We can use it in places where we know we can recover and GC must still be enabled.