Do not treat lack of retrieval of session data as an exception

[xsawyerx]

Right now we throw an error when we can't retrieve. Since not being able to retrieve does happen, we actually then test for it to not throw an error. This is broken.

[veryrusty]

At a quick look; I'd suggest:

_retrieve should return

• the hashref of data if successful,
• die on error if something went wrong (eg deserialisation of data fails), or
• just return if there was nothing to retrieve.

A quick scan of CPAN pointed that the DBIC and CGISession session classes are the only non-core session implementations that are likely to need updating for such a change. (Will need to do another check later..)

[demerphq]

re: veryrusty

FWIW, what you describe is what I would have expected, and sounds like a good change.

Yves

[xsawyerx]

I started looking into this and thought the first, simplest act would be to simply stop catching all retrieve calls again (since we're already catching inside retrieve all calls to _retrieve, the session retrieval method). Then I realized that means we won't handle any crashes from hooks, which are not guarded.

We're catching twice here because we want to be able to run the after hook, even if the retrieval failed. Then we need to catch again, to make sure we handle possible after hook crashes. But the weird part is that we do ignore any crashes of the session retrieval anyway.

I looked at handling of before/after of a route execution, in which case we do eval the before, we eval the route execution, but we do not eval the after. This suggests we want to run the route, even if we crash during the before, and we want to run the after, even if we crash in the route, but if we crash in the after, screw it, and make it crash everything.

If we want to have the same behavior here, we should remove the eval for the retrieve from Dancer2::Core::App. The _retrieval call within the retrieve method (inside Dancer2::Core::Role::SessionFactory) already has an eval, but the before hook doesn't have one, so we will need to add one there.

A question to @demerphq: All eval calls in Dancer2::Core::App call $EVAL_SHIM, but the evals in Dancer2::Core::Role::SessionFactory do not call it. Should they? What's the difference?