Hi, Damien. I have lots of comments on this issue, as I've just
implemented an exception-handling mechanism for the project I'm
working on. Some of the features you're considering could have made
my job a lot easier.
Hi, First of all, thanks for taking time to read and reply. And sorry to hear that timing could have been better. However, now we can use your experience to refine the implementation, which is a great benefit :)
Before I answer more precisely, I can tell you that the power-of-2 was an implementation idea thought as an alternative of OO inheritance, and it solved the "catch_exception should block other handlers" issue. However, after discussion with bigpresh and others, it occured to me that it was difficult to explain how to use these power-of-2 flags, and if it's troublesome for seasoned Dancer users, I guss it'll be unusable for newcomers. So I'll probably switch back to a more standard mechanism, like Exception::Base. But read below for the details.
CATCHING EXCEPTIONS
In the project that I'm working on, it would have been useful from a
plugin to catch an exception thrown in a route handler. A
"catch_exception" handler might have worked. But this isn't really a
hook, because if a "catch_exception" handler consumes an exception,
none on the other handlers in the chain should be called. It seems
to me, what we're talking about here is a new concept: a
"catch-point." It's like a hook in that it calls back to a handler
sub, but it more closely mirrors try-catch semantics.
Hmm, I think I see what you mean, however you may want the catching code to not stop execution of other handlers. So for me, your "catch_exception" handler can be implemented with a on_exception hook. The code in the hook closure can raise a new exception or a halt (which raises an exception too). Exceptions in hooks will break other handlers execution (see code in Dancer::Hook), and will be caught in Dancer::Handler::render_request. The D::Exception PR https://github.com/sukria/Dancer/pull/587 first purpose was to fix the fact that halt didn't break handlers chain. It now does. The "power-of-2" mechanism would allow a catching code to simply catch the exception, add E_HALT to it, and rethrow it. Thus the request would be immediately rendered as it were with halt, and the template would be able to use the original exception code and message. That's were it somehow replace the inheritance need. But as I said earlier, this mechanism is probably too complex to use.
So, my opinion is that your catch_exception is implementable with a after_exception hook, so it could fit nicely in the existing hook system. Please, if you see something that would make that statement invalid, let me know, as I'd rather tackle problems down now, rather than in 2 months :)
As you suggested, someone might also want to catch exceptions from
other parts of the process, e.g., exceptions thrown during
deserialization or serialization, or from within a hook. So it seems
that there are *at least* two distinct points at which applications
and plugins might want to catch exceptions:
* One is in $route->execute. A handler hooked into this
catch-point would execute in the context of the handler, and could
do anything the handler could do, including modify the response,
send errors, return serializable data, and throw the same or
different exceptions.
* The other is in Dancer::Handler::render_request. A handler
hooked into this catch-point would be able to modify the response,
including sending errors, but would not have access to the
serializer or router. This handler should also be able to throw the
same or other exceptions, to be caught by later handlers in the
chain.
And they may want to catch non-Dancer exceptions, too. Just as
Dancer has default handling in Dancer::Handler::render_request for
non-Dancer exceptions, so also should any catch-point mechanism
treat both Dancer and non-Dancer exceptions equally.
I don't see clearly why you'd need to catch exception at the two places. What is it you'd want to do in the execute catch_code that you couldn't do in the render_request catch_code ? Why couldn't render_request catch_code not change the serializer?
Also, if render_request (re-)throw exceptions, I'm wondering where/by who it would be caught ? except handle_request that is in the caller stack, above that you're pretty much out of Dancer's code. Maybe I don't understand what you mean ?
OTHER IDEAS
Idea 2, try-catch route syntax, doesn't add any capability that
wasn't implementable before. Might be nice for pretty syntax, but I
don't particularly care for it.
Agreed, that's what I stated in my email. So as soon as we show in the doc how to catch using eval {}; and suggest Try::Tiny, there is no point having a special syntax.
I'd rather just use Try::Tiny, then
in my route handler:
Idea 3, using the before_error hook, doesn't really seem like the
right solution, because this is about catching exceptions, not about
generating errors.
Agreed, I was referring at it in my email as a solution to avoid having an additional hook, by using before_error and shared data. But it's a hack and a bad idea.
Yes, by default, Dancer instantiates and renders
an error on a caught exception, but there's no particular reason why
that's the only way to handle an exception. In some cases, for
example, the right way to respond to a particular exception might be
to forward to a different path. Or to generate a non-error response.
And using shared data to communicate the exception is so very, very
wrong.
Completely agree. You'd be happy that the situation you describe is currently being taken care of, and someone is going to devote a lot of work on it very soon. I won't say more to avoid spoiling :)
THE SESSION SYSTEM EXAMPLE
I haven't used the Dancer session system, and I'm using the latest
released Dancer (not the devel branch). So I can't really
knowledgeably comment on "Real life example 3." But it seems to me
that this is a situation in which you'd want to use a meta-session
class, analogous to Dancer::Serializer::Mutable.
Ok yes :) But I was examinating the situation as "let's see how a user could implement this behaviour with just exception", so no additional module
That is, the
mutable serializer chooses which serializer to use— in that case,
based on the content types that the request and the serializers
support. Then Dancer::Serializer::Mutable proxies the API to that
chosen serializer.
One might visualize a session meta-engine that tries each configured
session engine, handing off control to that engine. As each engine
threw E_SESSION, the meta-engine would catch it, trying each
additional engine in turn, until it found one that accepted the
request. The meta-engine itself would only raise E_SESSION when none
of the configured engines could handle the request.
The meta-engine (if possible) could even cache the knowledge it
learned from these trials, so that it wouldn't have to go through
multiple trials and failures for each and every request. That sounds
very inefficient, multiple trials and failures every single request,
for normal, expected requests. It should have some way of knowing
which engine is to handle a given request, and funnel that request
to that engine. Only when an engine becomes inoperative (e.g.,
because a database has gone down) should it search for an
alternative.
In particular, you probably don't want to implement this
functionality by catching the E_SESSION exceptions at a higher
level. Because its logic is local to the session subsystem. Keep
things that work together close together; keep things that work
separately apart.
That's what I would do if I were to write a Dancer::MetaSession class indeed.
However, if there is a compelling reason to catch exceptions at some
other point in the Dancer framework, you could define additional
catch-points.
Additional thinking : there has been some proposals made that more of
the Dancer DSL should use exceptions as workflow, for instance,
'forward', 'redirect', etc, things that should immediately stop the route code execution should raise an exception. And be caught by the immediate route code caller, so that hooks and the rest of handler can continue to be properly executed. That btw would be a strong point in favor of having a catch-point around route execution. Implementation-wise, using the "power-of-2" system, it could be done by raising exceptions of type E_ROUTE & E_FORWARD, or E_ROUTE & E_REDIRECT for instance. And only E_ROUTE exceptions would be caught in the route catch-point.
DANCER EXCEPTIONS
Apps may want to throw other exception objects, not just internal
Dancer exceptions. And it's important to keep that in mind. Because
Dancer::Exception is of limited usefulness to many apps.
I agree with that, that's why render_response catches all exceptions, not only Dancer's. An a on_exception hook would also catch
Frankly, I'm still stymied by the choice to use a power-of-2 integer
as an exception code in Dancer::Exception. It's no more lightweight
than using an arbitrary integer—in fact, it's slightly heavier, as
seen below—and it restricts the usefulness of the class.
The reason to use a power-of-2 integer is if you have a small,
limited number of defined conditions, and you want to communicate
two or more of them simultaneously in the same value. So for
example:
use Fcntl ':mode';
$mode = (stat($filename))[2];
print "$filename is readable\n" if ($mode & S_IREAD);
print "$filename is a directory\n" if ($mode & S_IFDIR);
In this example, the mode returned by stat is a bit-mask
simultaneously communicating the state of all the different file
permissions that stat knows about, as they pertain to the $filename
being examined.
But exceptions are a horse of a different color... or maybe an
animal of a different species. You only raise one exception at a
time, and there can be an arbitrarily large set of possible
exceptions that an app might throw. Looking at the latest released
Dancer source, I see that Dancer::Exception will refuse even to
register more than 16 exceptions— on a system with 64-bit integers.
That means you're limited to a maximum of 16 different exception
types, and of the 64 bits carried around with each exception object,
48 of them will always go to waste. That's heavier (or less useful)
than using an arbitrary integer value.
While Dancer::Exception might still be useable for internal Dancer
exceptions, it fails to provide the minimum feature-set needed for
most application code. In general, a lightweight exception class
should encompass a simple integer value and a string, and it should
be subclassable for apps that need to extend it... Of course, what
I've just described is Exception::Base, which is why I've been
throwing Exception::Base objects in my code.
Here is the (maybe flawed) reasoning I had.
Traditionally, exceptions are implemented as you mentioned, as classes, like Exception::Base. Object inheritance is used to derive exceptions from a base one. But in cases of exceptions, inheritance is (99% of the time) only used to specify that an exception Foo is also from type Bar. Inheritance is not used for overloading, adding, removing / changing methods, or attributes.
So from my point of view, inheritance does the job of allowing mixing/subclassing exception, but it's like using a very big and complex tool to do soemthing relatively simple.
So the "flag" oriented implementation was a attempt to allow subclassing / mixing exceptions without the burden of OO.
For example, in OO, if you'd want to create an exception thrown by redirect, and that should be caught just around a route, you'd have to implement : Dancer::Exception (base class, at least caught at render_response )
Dancer::Exception::Route (exceptions that have to be caught around a route code execuction) Dancer::Exception::Route::Redirect
That's a lot of files and modules to write. Using "power-of-2" stuff, you just' just have E_ROUTE, E_REDIRECT.
That said, it seems that the current system is too complicated to grasp and use, and the 16 exception types may be a problem (I don't think it is, but it's a limitation nevertheless).
So, I think I'll rewrite the whole, with Dancer::Exception as base class, and classic OO inheritance.
No special catch syntax, we let users use eval {} or Try::Tiny. I'm not sure on the hooks yet. There is a need to provide users with to possibility to define hooks to catch exceptions from a route, and continue the workflow, and hooks to catch exceptions thrown from outside routes.
But that makes 2 hook types, which I'm not very happy with. Any idea ?
Anyhow, you asked for feedback. Hope you weren't disappointed. :)
I'm in the midle of doing the second round coding for
Dancer::Exception (it's in topic/exceptions2 on github). I have
few design questions, and I'd like some feedback from the
community.
The plan is that with Dancer::Exception :
- 1/ an exception has a value ( combination of power of 2
integers), and a message (string)
- 2/ all exceptions raised by the Dancer core will be
Dancer::Exception (instead of normal die / croak)
- 3/ a user will be able to raise exceptions in the routes
- 4/ a user will be able to create new exception types
- 5/ when raised, exceptions are transmitted to the templating
system when rendering a Dancer::Error
- 6/ exceptions should properly stringify and numerify for
backward compatibility and ease of use.
- 7/ the user should have the opportunity to catch any Dancer
exception and perform an appropriate action
Currently, in the topic/exceptions2 branch, all items are
implemented, except item 7. More precisely, it's possible to catch
Dancer::Exception using standard eval {...}. But what if a user
wants to catch an exception raised from a distant route, or from
Dancer Core itself ?
Real life example 1 : in some routes, the developer does some
'raise' of exceptions (internal or custom). The developer wants to
run specific codes when these exception are raised. For now, he
can wrap his route code with eval {} and use is_dancer_exception,
as stated in Dancer::Exception.
Real life example 2 : in some routes, the developper wants to
catch core exceptions that could occur.
Real life example 3 : if something wrong happens in the
Dancer::Session::Yaml modules, it raises a E_SESSION exception.
When this exception occurs, it'll render a Dancer::Error, stating
that thet exception were a E_SESSION. However what would be cool
is that you could catch the exceptions, see that it's of type
SESSION, and in this case try a different Dancer::Session system.
In this case the exception is a core one.
example 3 is not related to routes, so it's a bit difficult to
know where to put a try/catch scope.
I have some ideas, but I'm not really convinced by them :
- idea 1 : add a new hook, that would be called 'after_exception'
- idea 2 : when an exception is raised, it's stored as Shared
Data, and thus can be detected with a before_error hook
- idea 3 : add a try_catch syntax when creating route, like :
get '/foo' => sub { ... raise E_LOGIN, "bad credential" },
catch E_REQUEST | E_GENERIC => sub { ... }
catch E_LOGIN => sub { ... }
Currently, Dancer::Exception is not too bad : it's simple,
efficient and fast. I don't want it to become a bloated system.
But we need to provide a bit more feature for it to be really
useful.