Hi PHP
So I read that there's this Throwable interface coming. Great! How about
extending it with one further method:
void Throwable::addSuppressed(Throwable exception)
Semantic is the same as Java's Throwable.addSuppressed()¹.
Why? Well consider a code fragment which wants to close a resource
during an exception:
} catch (Exception $e1) {
try {
$resource->close();
throw $e1;
} catch (ResourceException $e2) {
// The information about $e2 is lost.
throw $e1;
}
}
Currently PHP has no method to propagate both $e1 and $e2. With
Throwable::addSuppressed() $e2 could be added as a suppressed exception
to $e1:
} catch (Exception $e1) {
try {
$resource->close();
} catch (ResourceException $e2) {
e1->addSuppressed($e2);
}
throw $e1;
}
To make this information useful (for e.g. a logger) there's one further
method needed:
Throwable[] Throwable::getSuppressed()
So PHP, what do you think, might a RFC find acceptance?
Best wishes
Markus Malkusch
Hi PHP
So I read that there's this Throwable interface coming. Great! How about
extending it with one further method:void Throwable::addSuppressed(Throwable exception)
Semantic is the same as Java's Throwable.addSuppressed()¹.
Why? Well consider a code fragment which wants to close a resource
during an exception:} catch (Exception $e1) {
try {
$resource->close();
throw $e1;} catch (ResourceException $e2) { // The information about $e2 is lost. throw $e1; }
}
Currently PHP has no method to propagate both $e1 and $e2. With
Throwable::addSuppressed() $e2 could be added as a suppressed exception
to $e1:} catch (Exception $e1) {
try {
$resource->close();} catch (ResourceException $e2) { e1->addSuppressed($e2); } throw $e1;
}
To make this information useful (for e.g. a logger) there's one further
method needed:Throwable[] Throwable::getSuppressed()
So PHP, what do you think, might a RFC find acceptance?
Best wishes
Markus Malkusch
Interesting thought, but how is this different than including a previous
throwable and throwing a new, more descriptive exception?
} catch (Exception $e1) {
try {
$resource->close();
} catch (ResourceException $e2) {
// Pass in $e2 as the previous exception in the chain.
throw new ResourceFailedException($e1->getMessage(),
$e1->getCode(), $e2);
}
}
What you're describing doesn't seem like a common use case.
Also, this is pretty nitpicky, but the above code might fit better with
a finally block when closing resources:
try {
// Do something with $resource here...
} finally {
$resource->close();
}
-- Stephen Coakley
Stephen Coakley:
Interesting thought, but how is this different than including a previous
throwable and throwing a new, more descriptive exception?
Thank you for asking. If you would have again a look at the code
fragment from the previous mail you will notice that there are two
immutable exceptions: $e1 and $e2. But only one exception may leave the
context. There is no possibility to attach $e2 to $e1, so some
information will get lost.
Also the scenario could be extended to n further exceptions, as n
further resources might need some clean up operations.
What you're describing doesn't seem like a common use case.
I'd argue it is. Just imagine there are remaining resources which you
really want to release in any case (e.g. releasing a lock). In the case
of an exception (from the code before the releasing), this exception
should leave the method plus the resources should be closed. Now what
happens if closing those resources cause further exceptions? Only one
exception may leave the method.
Also other languages (e.g. Python and Java) decided that this use case
is common enough.
I'd like to illustrate it with a further more verbose example:
public function withdraw($amount)
{
$this->lock();
try {
// Let's assume anything here can throw an exception.
$balance = $this->getBalance();
$balance -= $amount;
$this->saveBalance($balance);
$this->unlock(); // throws LockException
} catch (LockException $e) {
// Releasing failed, so nothing else to do and that's the only
// exception, so we're good here.
throw $e;
} catch (\Exception $e1) {
// unlock() was not called yet.
assert ($this->isLocked());
try {
$this->unlock();
// The resource was released and we can throw $e1.
throw $e1;
} catch (LockException $e2) {
// Now I can only throw $e1 or $e2, that's why I add $e2.
$e1->addSuppressed($e2);
throw $e1;
}
}
}
the above code might fit better with a finally block when closing resources:
try {
// Do something with $resource here...
} finally {
$resource->close();
}
Finally doesn't address this use case. With finally I would just
exchange any origin exception with a possible exception from the close()
operation.
But I agree that try {} catch {try {catch {}} is quiet a huge boiler
plate. That's why Java introduced together with
Throwable::addSupressed() the try-with-resource statement. I would also
love to see try-with-resource here, but for the beginning I'd like to
focus on Throwable::addSupressed().
So do you still see no use case?
Markus Malkusch
Stephen Coakley:
Interesting thought, but how is this different than including a previous
throwable and throwing a new, more descriptive exception?} catch (Exception $e1) {
try {
$resource->close();
} catch (ResourceException $e2) {
// Pass in $e2 as the previous exception in the chain.
throw new ResourceFailedException($e1->getMessage(),
$e1->getCode(), $e2);
}
}
Sorry, in my previous mail I actually forgot to answer your question.
What you suggest is a common pattern to work around the lack of
Throwable::addSupressed(). There are two big differences:
-
There's no causality between e1 and e2. You pass the message from e1
to the newly created ResourceFailedException which is caused by e2.
Actually the causality here is correct, as ResourceFailedException is
caused by e2. But what I understand is that ResourceFailedException
should substitute e1 and e1 is not caused by e2. Just imagine e1 has the
message "insufficient funds". Then the user gets an exception which says
"insufficient funds" is caused by "could not rollback transaction". -
You are loosing one stack trace (in this case from e1). For me the
stack trace is the most important information in the exception. In this
use case there are two stack traces and I want to have both of them.
And then again this is just a simple example scenario with one resource.
There might exist n resources.
Markus Malkusch
Stephen Coakley:
Interesting thought, but how is this different than including a previous
throwable and throwing a new, more descriptive exception?} catch (Exception $e1) {
try {
$resource->close();
} catch (ResourceException $e2) {
// Pass in $e2 as the previous exception in the chain.
throw new ResourceFailedException($e1->getMessage(),
$e1->getCode(), $e2);
}
}Sorry, in my previous mail I actually forgot to answer your question.
What you suggest is a common pattern to work around the lack of
Throwable::addSupressed(). There are two big differences:
There's no causality between e1 and e2. You pass the message from e1
to the newly created ResourceFailedException which is caused by e2.
Actually the causality here is correct, as ResourceFailedException is
caused by e2. But what I understand is that ResourceFailedException
should substitute e1 and e1 is not caused by e2. Just imagine e1 has the
message "insufficient funds". Then the user gets an exception which says
"insufficient funds" is caused by "could not rollback transaction".You are loosing one stack trace (in this case from e1). For me the
stack trace is the most important information in the exception. In this
use case there are two stack traces and I want to have both of them.And then again this is just a simple example scenario with one resource.
There might exist n resources.Markus Malkusch
That makes sense -- I can see the uses for that. I don't mean to play
devil's advocate, but is it worth sacrificing the immutability of
exceptions for an addSupressed() method? Other than that, I think I
would be for such an addition.
--
Stephen Coakley
Markus Malkusch:
- You are loosing one stack trace
I'd like to revise that. I'd just learned that finally does indeed fit
here, as it would automatically glue exceptions:
try {
throw new Exception("A");
} finally {
throw new Exception("B");
}
This prints both exceptions:
PHP Fatal error: Uncaught exception 'Exception' with message 'A' in
/home/malkusch/tmp/test.php:4 Stack trace:
#0 {main}
Next exception 'Exception' with message 'B' in /home/malkusch>
/tmp/test.php:7 Stack trace:
#0 {main}
So B's previous became A. Well I'm surprised, as there's no causality
between them, but OTOH it's called Exception::getPrevious() without any
further semantics. So this might be acceptable.
Unfortunately this reduces the justification for
Throwable::addSupressed() only to catch blocks.
Markus Malkusch
Markus Malkusch wrote:
Markus Malkusch:
- You are loosing one stack trace
I'd like to revise that. I'd just learned that finally does indeed fit
here, as it would automatically glue exceptions:try {
throw new Exception("A");} finally {
throw new Exception("B");
}This prints both exceptions:
PHP Fatal error: Uncaught exception 'Exception' with message 'A' in /home/malkusch/tmp/test.php:4 Stack trace: #0 {main} Next exception 'Exception' with message 'B' in /home/malkusch> /tmp/test.php:7 Stack trace: #0 {main}
So B's previous became A. Well I'm surprised, as there's no causality
between them, but OTOH it's called Exception::getPrevious() without any
further semantics. So this might be acceptable.
Note that there's an open bug report (https://bugs.php.net/68270)
regarding this issue.
--
Christoph M. Becker
Christoph Becker:
Markus Malkusch wrote:
I'd like to revise that. I'd just learned that finally does indeed fit
here, as it would automatically glue exceptions:
[..]
Note that there's an open bug report (https://bugs.php.net/68270)
regarding this issue.
Thank you for pointing to that bug. So I understand the authors of that
bug are surprised that finally does magically attach exceptions. I did
actually also not expect that, as it is not documented nor do I have
that experience from other languages (did only check with Java).
So I can assume that magic finally glue behaviour is not considered
stable, which then again gives Throwable::addSupressed() IMO a
justification.
Markus Malkusch
Christoph Becker:
Markus Malkusch wrote:
I'd like to revise that. I'd just learned that finally does indeed fit
here, as it would automatically glue exceptions:[..]
Note that there's an open bug report (https://bugs.php.net/68270)
regarding this issue.Thank you for pointing to that bug. So I understand the authors of that
bug are surprised that finally does magically attach exceptions. I did
actually also not expect that, as it is not documented nor do I have
that experience from other languages (did only check with Java).So I can assume that magic finally glue behaviour is not considered
stable, which then again gives Throwable::addSupressed() IMO a
justification.Markus Malkusch
In that case, an addSupressed() method still makes sense, and is once
again a good idea. The finally block bug should be fixed before stable
release, too, I think.
--
Stephen Coakley
Christoph Becker:
Markus Malkusch wrote:
I'd like to revise that. I'd just learned that finally does indeed fit
here, as it would automatically glue exceptions:[..]
Note that there's an open bug report (https://bugs.php.net/68270)
regarding this issue.Thank you for pointing to that bug. So I understand the authors of that
bug are surprised that finally does magically attach exceptions. I did
actually also not expect that, as it is not documented nor do I have
that experience from other languages (did only check with Java).So I can assume that magic finally glue behaviour is not considered
stable, which then again gives Throwable::addSupressed() IMO a
justification.Markus Malkusch
So what should be the desired behavior, in regard to bug #68270? One
possible behavior would be to, if an exception is thrown inside a
finally block, to attach the uncaught exception from the try block via
the addSupressed() method. The last exception is thrown, and the
exception in the try block is still not lost. Such an alternative could
be bundled with the RFC.
The alternative s to just drop the exception, never to be seen again.
This is the behavior in both Java and C#. Python has a different
approach. Python has two properties that contain previous exceptions:
context, which holds implicitly chained exceptions, and cause,
which holds explicitly chained exceptions. This system makes a ton of
sense, and I think PHP exceptions should distinguish between the two as
well.
(Relevant PEP: https://www.python.org/dev/peps/pep-3134/)
Currently, in the try/finally situation described, $previous is used to
hold the missing exception. $previous should only hold explicitly passed
exceptions, as described in the docs. addSupressed() could be defined as
for implicit chained exceptions. The method could be paired with a
Throwable::getSupressed() to retrieve the implicitly added exception.
So in a case like this:
try {
try {
throw new Exception("Exception 1");
} finally {
throw new Exception("Exception 2");
}
} catch (Throwable $e) {
var_dump($e->getSupressed());
}
we would have $e be exception 2, and $e->getSupressed() return exception 1.
The more I think about it, the better the idea sounds. Good thought, Markus!
--
Stephen Coakley
Stephen Coakley:
So what should be the desired behavior, in regard to bug #68270? One
possible behavior would be to, if an exception is thrown inside a
finally block, to attach the uncaught exception from the try block via
the addSupressed() method. The last exception is thrown, and the
exception in the try block is still not lost. Such an alternative could
be bundled with the RFC.
I also like that idea very much that the try-exception is not lost in
this case. So I would agree defining this behaviour can be part of the
RFC as well. Also the current behaviour is nowhere documented so
changing it can be considered as backward compatible (plus it would
close bug #68270).
try {
try {
throw new Exception("Exception 1");
} finally {
throw new Exception("Exception 2");
}
} catch (Throwable $e) {
var_dump($e->getSupressed());
}we would have $e be exception 2, and $e->getSupressed() return exception 1.
Ack. But the original idea was that Throwable::getSupressed() will
return a list of exceptions. You probably didn't change that
intentionally. The try-finally scenario involves only one supressed
exception. I'd like to see a possibility to add more supressed
exceptions, as described in the initial mail for e.g. a use case where
just more resources need to be closed.
Now that try-finally is part of the scope as well, I'd like to add the
case that a supressed exception might be thrown from a catch block as well:
try {
throw new Exception("Exception 1");
} catch (Exception $e) {
throw new Exception("Exception 2", 0, $e);
} finally {
throw new Exception("Exception 3");
}
So in this case the resulting exception would be exception 3, which has
exception 2 as a supressed exception. Exception 1 is as already defined
the previous exception of exception 2.
The more I think about it, the better the idea sounds. Good thought,
Good to hear that the idea finds acceptance. Also I like the evolving of
it. I guess drafting a RFC might be an option then. I will let this idea
brew a bit and then start with that, assuming doing so is fine.
Markus Malkusch
Markus Malkusch:
The try-finally scenario involves only one supressed
exception. I'd like to see a possibility to add more supressed
exceptions
Actually try-finally can already involve several supressed exceptions as
well:
try {
throw new Exception("Exception 1");
} finally {
try {
throw new Exception("Exception 2");
} finally {
throw new Exception("Exception 3");
}
}
So in this scenario I exception 3 will be the resulting exception and as
already defined within your mail exception 2 should be its supressed
exception.
But what about exception 1? This could now be the supressed exception of
exception 2 or added to the list of supressed exceptions of exception 3.
I'd argue adding it to the resulting exception of the finally block
makes the most sense. This would be exception 3 in this case and
therefore needs to be a list. Why does this make more sense. I'd argue
it's more expected and intuitive, as exception 2 is optional. Just
consider a similar case without exception 2:
try {
throw new Exception("Exception 1");
} finally {
try {
} finally {
throw new Exception("Exception 3");
}
}
In this case the only place for the suppressed exception 1 can be
exception 3. For consistency and simplicity I'd therefore suggest that a
Throwable has a list of supressed exceptions and a suppressed exception
is always added to the resulting exception of a finally block.
Markus Malkusch
Markus Malkusch:
try {
throw new Exception("Exception 1");} finally {
try {} finally { throw new Exception("Exception 3"); }
}
In this case the only place for the suppressed exception 1 can be
exception 3. For consistency and simplicity I'd therefore suggest that a
Throwable has a list of supressed exceptions and a suppressed exception
is always added to the resulting exception of a finally block.
Actually I my self was not really certain why adding it to the resulting
exception is better than setting it to the first exception. So I'd like
to present a further scenario which hopefully helps to decide for the
better definition:
try {
throw new Exception("Exception 1");
} finally {
try {
throw new Exception("Exception 2");
} catch (Exception $e) {
// Don't throw anything cause this is expected.
} finally {
throw new Exception("Exception 3");
}
}
So in this scenario again exception 3 will be the resulting exception.
exception 2 is expected and caught. Regarding exception 1 consider these
two definitions:
-
The supressed exception will be attached to the first raised
exception. This would be exception 2 in this case. Exception 2 is
handled and the information about exception 1 is lost. -
The supressed exception is added to the resulting exception of the
finally block. This would be exception 3 in this case. The information
is still present.
So therefore I would vote for definition two. But I am also curious on
other opinions.
Markus Malkusch
Stephen Coakley:
So what should be the desired behavior, in regard to bug #68270? One
possible behavior would be to, if an exception is thrown inside a
finally block, to attach the uncaught exception from the try block via
the addSupressed() method. The last exception is thrown, and the
exception in the try block is still not lost. Such an alternative could
be bundled with the RFC.I also like that idea very much that the try-exception is not lost in
this case. So I would agree defining this behaviour can be part of the
RFC as well. Also the current behaviour is nowhere documented so
changing it can be considered as backward compatible (plus it would
close bug #68270).try {
try {
throw new Exception("Exception 1");
} finally {
throw new Exception("Exception 2");
}
} catch (Throwable $e) {
var_dump($e->getSupressed());
}we would have $e be exception 2, and $e->getSupressed() return exception 1.
Ack. But the original idea was that Throwable::getSupressed() will
return a list of exceptions. You probably didn't change that
intentionally. The try-finally scenario involves only one supressed
exception. I'd like to see a possibility to add more supressed
exceptions, as described in the initial mail for e.g. a use case where
just more resources need to be closed.Now that try-finally is part of the scope as well, I'd like to add the
case that a supressed exception might be thrown from a catch block as well:try {
throw new Exception("Exception 1");} catch (Exception $e) {
throw new Exception("Exception 2", 0, $e);} finally {
throw new Exception("Exception 3");
}So in this case the resulting exception would be exception 3, which has
exception 2 as a supressed exception. Exception 1 is as already defined
the previous exception of exception 2.The more I think about it, the better the idea sounds. Good thought,
Good to hear that the idea finds acceptance. Also I like the evolving of
it. I guess drafting a RFC might be an option then. I will let this idea
brew a bit and then start with that, assuming doing so is fine.Markus Malkusch
Ah, yes. I didn't realize that was the intention, but
Throwable::getSupressed() could return an array.
--
Stephen Coakley
Stephen Coakley:
This is the behavior in both Java and C#. Python has a different
approach. Python has two properties that contain previous exceptions:
context, which holds implicitly chained exceptions, and cause,
which holds explicitly chained exceptions. This system makes a ton of
sense, and I think PHP exceptions should distinguish between the two as
well.(Relevant PEP: https://www.python.org/dev/peps/pep-3134/)
Also thank you for that reference. As far as I understand this differs
in two aspects:
-
context is a single exception opposed to having a list of
suppressed exceptions. This probably needs some more discussion to
decide the approach. Although I would prefer having a list of suppressed
exceptions I could imaging that as well.
A list of suppressed exceptions would make things like this possible:
} catch (Exception $e1) {
foreach ($resources as $resource) {
try {
$resource->close();
} catch (Exception $e2) {
$e1->addSuppressed($e2);
}
}
throw $e1;
}
And just in case if in some distant future one might propose something
like Java's try-with-resource PHP can easily do so without any BC break.
- Caught exceptions will be considered as context as well. I'd say PHP
should not adopt that behaviour. First I'd like to understand the
semantic of suppressed as "a not caught exception". In Pythons world
it's called context, and therefore acceptable. But more than that this
leads to things like this:
try:
raise Exception("exception 1")
except Exception as e:
raise Exception("exception 2") from e
In this case exception 2 has exception 1 both as context and cause. I
think we don't need that information as we explicitly know by catching
the exception already the context.
Markus Malkusch