Bug #75056 [Opn->Csd]: Unintuitive gotcha - Exception maintains handles to stack objects

Edit report at https://bugs.php.net/bug.php?id=75056&edit=1 ID: 75056 Updated by: nikic@php.net Reported by: nachms+php at gmail dot com Summary: Unintuitive gotcha - Exception maintains handles to stack objects -Status: Open +Status: Closed Type: Bug Package: *General Issues PHP Version: 5.6.31 -Assigned To: +Assigned To: nikic Block user comment: N Private report: N New Comment: PHP 7.4 comes with an ini option to disable argument collection: zend.exception_ignore_args. The shipped ini files recommend enabling it in production and disabling in development. Previous Comments: ------------------------------------------------------------------------ [2017-08-12 23:19:51] nachms+php at gmail dot com I'm not familiar with the mailing list, but if you start a discussion there, I can try to join in. ---------------------- While I agree that serialization of exceptions can be very useful for keeping track of errors that occurred and other things, this aspect of holding onto objects can cause some really really bad issues. Take this example: foreach ($somethings as $something) { echo 'Processing ', $something, "\n"; try { do_something($something); } catch (Exception $e) { echo 'Error occurred with ', $something, "\n"; } } At first glance this code looks perfectly fine, right? But if somewhere inside do_something() there's an object controlling resources that ends up part of a call frame, no matter how nested, if there's something which ends up creating a cursor on a database or has an open network handle, and then encounters an error, that cursor or network handle will remain open till the next error occurs! No destructors will fire. Holding open a database cursor indefinitely can cause further database actions to go wrong. Leaving open a network handle may have security ramifications, or lead to file descriptor starvation. Correct code is actually: foreach ($somethings as $something) { echo 'Processing ', $something, "\n"; try { do_something($something); } catch (Exception $e) { echo 'Error occurred with ', $something, "\n"; unset($e); } } Without the unset, the Exception lives throughout the duration of the loop since PHP variables have function lifetime not block scope lifetime. The Exception will not die till the next error occurs and the Exception variable in the catch ($e) is overwritten. The Exception in turn keeps resources locked, and living far outside their scope, thereby breaking encapsulation. Typically you expect stack unwinding to enforce function lifetime, but with Exceptions, many variables not only have function lifetime but Exception lifetime! We ran into this issue the other day, where the lack of an unset had severe ramifications, and it took a team of our developers over 5 hours to track down the bug. I don't think it's the kind of thing any developer is expecting. We've since updated our PHP guidelines to always put an unset into exception handling code where the Exception is not then further used beyond. For cases where there's some master object keeping track of Exceptions for some reason, it's not actually safe for the loop to continue, because desctructors not firing to release managed resources prevents another iteration from working properly. Basically we discovered with PHP that Exception tracking + loops + resource objects that get passed around is a recipe for disaster, and there does not seem to be any viable solutions to do this in PHP as it currently works. This prevents us from using PHP for various long lived tasks. Typically you want exception handling to make error handling easier and prevent bugs. But it seems with PHP's designs, once you realize how it works, it can make error handling much harder and create all kinds of bugs if not dealt with properly. ------------------------------------------------------------------------ [2017-08-11 20:42:23] rowan dot collins at gmail dot com This was actually one of the first topics I raised on the PHP Internals mailing list (which is generally a better place for open-ended discussions like this - http://php.net/mailing-lists.php). That was nearly 4 years ago, and didn't get much response; it then came up again a couple of years later, in the context of serialization errors (which are caused by the same data). See these threads in the archives: - http://marc.info/?t=138118341600002&r=1&w=2 - http://marc.info/?t=142708828500001&r=1&w=2 - http://marc.info/?t=143798102800002&r=1&w=2 The peculiar thing is that most of the information stored for the backtrace is already reduced to strings (class names and the like), but the *arguments* passed to functions in the trace are preserved as full object references. My inclination remains that removing these object references would solve both the destructor and serialization problems, and I'm going to raise it on the list again. I invite you to join the discussion there. ------------------------------------------------------------------------ [2017-08-09 15:05:07] nachms+php at gmail dot com Description: ------------ Consider the attached code, it outputs: The throw did not destruct destructing -------------- This is somewhat surprising at first, but is due to exceptions containing a backtrace of all function parameters keeping objects alive somewhat unintuitively. This really means that to have sane destruction behavior with exception handling, one must always unset the Exception at the end of a catch. Which is usually fine... However, if one needs to rethrow from an exception handler, this means objects that need to be destructed as early as possible are now long-lived through many nested levels. There is no built-in way in PHP to remove these handles from the exception's backtrace. With the way PHP currently works, we've found that an exception to handle some occasional error could in turn generate a whole chain reaction of problems simply because objects are now living far outside their scope, holding onto resources they ideally should not. We're not sure of the best solution here, but it would seem that in order to handle these kinds of cases, PHP needs to offer a way to catch exceptions without keeping handles to stack objects. Perhaps some other way to catch, or some way to tell Exceptions to drop object handles from their backtrace before further handling them. Test script: --------------- <?php $destructed = false; class c { function __destruct() { global $destructed; $destructed = true; echo 'destructing', "\n"; } } function thrower($c) { throw(new Exception('oops')); } function test() { $c = new c(); thrower($c); } try { test(); } catch(Exception $e) { echo 'The throw ', ($destructed ? 'destructed' : 'did not destruct'), "\n"; } Expected result: ---------------- One would intuitively expect: destructing The throw destructed ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=75056&edit=1