Thanks for the detailed reply Zeev.
On Tue, Dec 10, 2013 at 1:55 PM, Zeev Suraski <zeev@zend.com> wrote:
> Nikita, Philip, see below:
>
> > On Mon, Dec 9, 2013 at 11:41 AM, Nikita Popov <nikita.ppv@gmail.com>
> > wrote:
> > > You can't convert fatal errors to exceptions using an error handler.
> > > That's why I'm targeting fatal errors here and not all errors in
> > > general. While I'm not a big fan of PHP's error system in general,
> > > it's easy enough to turn warnings or notices into exceptions via error
> > > handlers. But for fatal errors that's just not possible.
>
> That's correct, but you seem to be ignoring the reason for that. It's not
> an arbitrary decision, but rather, the semantics of what fatal errors are.
> They're fatal, not recoverable (with the reason being that the
> engine/extension/whatever may be in an unstable state), and therefore it
> should not be possible to use *any* mechanism to recover from them, be
> them error handlers or exceptions.
>
Ostensibly, yes. Practically that's just not true. Most (nearly all) fatal
errors occur in situation where there is no global engine instability, but
only some local *inconvenience*. Like, I tried to fetch this zval, but got
NULL instead (because it's a string offset), what do I do now? No
instability, just not pleasant to continue from here.
I have already ported approximately 40% of the fatal errors in the engine
to use exceptions. I can assure you that all of those have nothing to do
with engine instability and changing them to exception was mostly trivial
(the hard part is adjusting phpt outputs...)
I'm pretty sure that most of the remaining ~100 fatal errors will be just
as easy to convert. I just looked through the list to find the ones which
*can't* be converted:
* "Invalid opcode %d/%d/%d" => This error only occurs in case of a serious
skrewup from our side. Shouldn't occur in practice
* "Arrived at end of main loop which shouldn't happen" => This is
effectively dead code, just a debugging message.
* "Balloc() failed to allocate memory" and "Balloc() allocation exceeds
list boundary" => Those two are from zend_strtod. Again, you won't see them
in practice (unless you try to parse a 100K digit float maybe. don't know
the exact background here)
* "Error installing signal handler for %d" => From zend_signals, occurs
when a sigaction call fails. This seems like a "just to be sure" check that
shouldn't occur in practice. But in either case, this is only relevant for
pcntl, as nothing else makes use of the function.
* "Attempt to destruct pending exception" => (Not sure whether this is
just a debugging error or whether it can really occur. We don't have a test
for it at least)
* "Method %s::__toString() must not throw an exception"
* + 6 more exception related fatals (like exception must be object etc) =>
may be problematic to use exceptions for errors about exceptions (but I may
be wrong here, it might well be that exceptions are safe here)
* "Nesting level too deep - recursive dependency?" => from zend_hash. As
this is used in a bunch of places, likely can't be changed.
* "Class entry requested for an object without PHP class" => Doesn't occur
in practice, because we always use objects with ce.
* "Allowed memory size / Out of memory" => The obvious candidates ^^
* "Possible integer overflow in memory allocation"
* "Maximum execution time of %d second%s exceeded"
* "Corrupted fcall_info provided to zend_call_function()" => Once again,
this one doesn't occur in practice, just debugging.
That list has 17 errors that can't or likely can't be converted. Add 10
more and round up to 30 to account for stuff I might have missed. This
still allows us to convert approximately 85% of the errors. Also note that
at least 6 of those are really "debugging errors" that do not practically
occur. The most common ones from the list will be the "Allowed memory size"
and "Maximum execution time" errors, but we knew from the start that those
can't be changed.
To also approach this from the other side, here are some examples of errors
that I have already converted to exceptions (listing everything would blow
things up a bit too much):
* "Call to a member function %s() on a non-object" => Arguably the error
that annoys people most
* "Call to undefined method %s::%s()", "Call to undefined function %s()",
"Class '%s' not found" + variations on the "not found" topic
* "Cannot call abstract method %s::%s()" + a number of variations on the
"cannot call" topic
* "Class name must be a valid object or a string", "Method name must be a
string" + variations on the "must be xyz" topic
* "Cannot use string offset as an array" + a huge number of variations on
the same thing. We have a lot of string offset related fatals ;)
* "Cannot use [] for reading"
* "Cannot pass parameter %d by reference"
* "Cannot instantiate %s %s"
* ...
Hopefully you can now see that fatal errors - in most cases - do not cause
engine instability and can be relatively easily recovered. Of course, some
fatal errors will stay, no way around that. But we *can* convert a good
portion of them. And many of the errors that are actually important to
people (like "Call [...] on a non-object") can be (and are already) easily
converted.
> > To achieve what I want it would be enough to turn fatal errors into
> > > recoverable fatal errors, that would already allow the error handler
> > > approach. But that's a lot harder to do (and imho less usable) than
> > > directly using exceptions. Keeping the engine stable after an
> > > exceptions is relatively simple, the same can't be said about
> recoverable
> > fatals.
>
> While you pointed out in the RFC a specific example of a place where an
> exception would be a bit easier to implement than an error, it's a very
> specialized example - errors that happen on the void between two function
> scopes - and doesn't hold true for most errors. I don't think it makes
> sense to say it's significantly easier to turn fatal errors into
> exceptions than turning fatal errors into recoverable errors.
>
Again, this just isn't true. If I say that converting fatals to recoverable
errors is harder, I say that because I actually tried to do it before going
with the exception approach. For exceptions the procedure is always the
same: Free resources, then return (+ ensuring exception safety in the
calling code, but that's usually a given). For recoverables things are a
lot more complicated. Let's forget about the complex cases where function
calls and stacks are involved, let's just take the very simplest fatal
errors like "Cannot use string offset as an array". Those types of fatals
occur whenever a GET_OPx_ZVAL_PTR_PTR call returns NULL. As said, with
exceptions handling them is trivial, but what do you do in the case of
recoverable? Do you allocate a new IS_NULL zval and work on that? Of course
that would just produce a bunch of new warnings or errors down the row
(also just allocating a zval may not be enough for the PTR_PTR case). It
would be a lot better to make the result of the "whole" operation (rather
than an individual fetch) an IS_NULL zval (without additional errors), but
that's where things will get technically *very* complicated.
With recoverables every single errors needs careful consideration to find
some behavior where we can continue execution in some
not-totally-meaningless way. With exceptions you just always follow the
same dumb scheme ;)
> Secondly, the patch radically changes execution behavior in case of
> recoverable errors for people using error handlers and arguably in a very
> bad way, effectively forcing them to start using exceptions if they want
> to truly recover from the error. Otherwise, by the time the exception
> turns into an error, there's nothing you can recover (you've already
> bubbled up all the way to the global scope). That will break apps relying
> on the current behavior.
>
Yes, there are concerns regarding E_RECOVERABLE_ERROR, that's why I added a
separate voting options for it. If recoverable errors are your main issue
(which seems so, given that most of your mail was about that), then that's
your voting option ;)
> Last, in the RFC it mentions that 'recoverable errors are hard to catch'.
> That you're forced to use an error_handler. How are they any different
> from any other error/warning type?
Not any different. Please realize though that "Just use an error handler to
turn everything into exceptions" is a somewhat idyllic view that fails when
it comes to library inter-compatibility. If you are writing portable code
you can't use a global error handler to convert everything to exceptions.
Instead you need to create and restore an error handler in every single
place where you want to make use of try/catch with a recoverable ;)
Nikita