Bug #65196 for DBIx-Connector: Try::Tiny catch is not catching exceptions

On Mon Jan 31, 2011 at 12:41:00PM -0500, David Wheeler via RT wrote: Show quoted text
$conn->txn( no_ping => sub { die 'WTF!'; }, catch => sub { warn "Caught $_"; }, ); # Or more often because subs are large $conn->txn( no_ping => sub { die 'WTF!'; }, catch => sub { warn "Caught $_"; } ); So after using "catch => sub" a few times with nice indentation the annoyance has gone away, and the readability is still quite good. If not better. In fact, now that I've been using this I notice that it is more consistent when both subroutines are specified as "sub {...}" instead of one of them being a special keyword. I would not miss the Try::Tiny integration if you decided to remove that feature. What is nice though about Try::Tiny (since I don't wanting to throw the bathwater out completely) is when you can use it fully: The "try {} catch {}" grouping looks great, and is *almost* duplicated in DBIx::Connector. So what do you think about the following syntax? $conn->txn( try => sub { die 'WTF!'; }, catch => sub { warn "Caught $_"; } ); Most of the time my interest in the code is about *my* actions, not what DBIx::Connector is going to do under the hood when things fail. I generally set $conn->mode and forget about it, and the above syntax I think puts the focus in the right place. To extend the above idea further one could use "try_fixup", "try_no_ping" etc for the one-off change to the mode. The good news is that these could be added without affecting the current syntax. Show quoted text
Yes. But see below. Show quoted text
I couldn't narrow the problem down and now it has gone away - not on it's own of course, I've been doing lots of re-factoring of the calling code. I'm going to write it off as something on my side. Cheers, Mark. -- Mark Lawrence

On Thu Mar 17, 2011 at 02:55:45PM -0400, David Wheeler via RT wrote: Show quoted text
But I didn't give strong enough reasoning for you to add it... Show quoted text
Although you were referring to your examples, the 'meaning' behind 'try' in mine above is not a no-op. It actually would stand for "use the current default mode when 'trying'". Show quoted text
It does look best when balanced but being explicit is the only way of doing so. If you want to default to the current mode (which lets you make a global change without changing all your code) you stay unbalanced. I've never quite liked interfaces that took an even and odd number of arguments... but it's your bikeshed :-) Mark. -- Mark Lawrence

On Tue May 10 12:12:52 2011, DWHEELER wrote: Show quoted text
Nevertheless, looking at Try::Tiny, that does seem to be the expectation. Its docs say: # just silence errors try { die "foo"; }; Looking at the code, I think I could actually make that happen pretty easily. However, it would then also imply that you should be able to do `try => fixup`, because `fixup` does not imply `try`. And I think that syntax looks awful. So I see a few possible solutions: 1. Use some term other than `try` to mean "the default mode". Suggestions welcome (not `default`; ew). 2. Use `try`, make it swallow exceptions if there's no `catch`, and modify the other params to imply `try`. I don't think this is a good idea for compatibility reasons. 3. Use `try`, make it swallow exceptions if there's no `catch`, and add `try_fixup`, `try_ping`, and `try_no_ping`. Not really keen on those parameters, though. The trouble is that the use of `catch` has *implied* `try`, which probably wasn't the best idea. Hell, adding `catch` at all probably wasn't the greatest idea; one should probably just use Try::Tiny and not bother with it in DBIx::Connector (should never have been DBIC's domain). Anyway, just some thoughts. I'll think about it some more and make a decision soonish. David

Wow, lots of thoughts, both here and on your blog. I'm somewhat sorry for opening up this can of worms. On Tue May 10, 2011 at 12:23:16PM -0400, David Wheeler via RT wrote: Show quoted text
( plus your 3. from the blog: depreciate try/catch altogether ) Show quoted text
I must admit I'm starting to lose track of all the different possible contexts in which the try/fixup/ping/noping/catch keywords apply. I think this is partly due to the mixing of several types of exceptions, and how they can be handled. For my own benefit (I know you already know this) I'll re-word the problem: We have Standard Perl Exceptions: Can be raised in user code, or by DBI/DBD (assuming RaiseError) at any time. Before DBIx::Connector the calling code has to handle this on their own with explicit calls to begin_work, commit and rollback. Either with "eval/if $@" or Try::Tiny try/catch: try { begin_work <work> commit } catch { rollback <handler> } Messy to write and messy to try and find out if the error was generated by SQL, Perl, or network/connection, so (most?) people treat all errors the same way? DBIx::Connector adds two nice things relevant to this discussion: - Encapsulates the begin_work/commit/rollback/code into a single method call - An internal catch/retry handler for connection-failures In my opinion these are the main reasons for using DBIx::Connector. Both of these features involve a form of exception handling. The questions I see in front of me are: 1. Whether to integrate user exception-handlers into the DBIx::Connector method calls; 2. If the answer to the above question is yes, then how to integrate the user-handler code with existing connection-failure mode keywords. Ok, now I've re-written what you've already said and not really moved anything forwards. But at least I can think more clearly :-/ Here are my thoughts on your blog post options: 1. "default" keyword doesn't give me a warm feeling and I can't think of anything better. 2. I sort of like the "try" keyword (I should since I suggested it :-) and I don't find the try_fixup thing very ugly, but I also never use the per-statement mode keywords. I agree about the behaviour change not being ideal. 3. I don't like the idea of going back to exception handling outside of the run/txn/svp calls. If DBIx::Connector is going to call rollback for me I want to let it call my handler as well. In other words either make everything explicit, or make everything implicit. Here is a fourth suggestion. Depreciate the 'catch' keyword in the existing run/txn/svp calls (or let it keep its current behaviour) and introduce new *methods* that make the handling explicit: Keep current behaviour with no (or external) user-handlers: $conn->run(sub{ # can die and user program stops, but will re-run/fixup # depending on the default mode }); try { $conn->txn(no_ping => sub{ # can die and user program stops, will not ping because # explicitly set }); } catch { # do our own thing } # etc etc Add new behaviour: # never dies (swallows exceptions) but retries according to default # mode $conn->try_txn(sub{ # stuff which can die }); $conn->try_txn(ping => sub{ # stuff which can die }, catch => sub { # handle the non-connection exceptions }); $conn->try_svp(fixup => sub{ # as above }, catch => sub { # as above }); The above variations make things explicit where they need to be, follow the same pattern as try/eval, and keep existing compatibility. I was going to say I much prefer not having the extra indentation without external try/eval but I'm now thinking asthetics should come waaay after any others discussions have finished, so that's not really an argument. Likewise the importantance to me of "balance" has diminished since we last spoke about that issue. Hope this helps. Mark. -- Mark Lawrence

On May 11, 2011, at 5:33 PM, Mark Lawrence via RT wrote: Show quoted text
Yes, but you don't actually need to know that or even be concerned with it. You happen to know that because you used to write the code the old way, not because there's anything about the API to suggest it. Show quoted text
No worries. I've talked to a bunch of people about this, and so far, with one exception, their advice has been to go with #3 from my blog post: rip out the exception handling. So I think that's what I'll do. I'm thinking: 1. Remove all documentation of it, except for one spot that says it's deprecated, and release that change. 2. A month later, release a new version that warns when a catch block is passed. I'd set it up to only warn once for each caller so that logs don't fill up. 3. A month after, that, release a new version with the catch stuff completely removed. Would that be sufficient for you? I have code that needs to change, too… Best, David

On May 11, 2011, at 8:07 PM, Mark Lawrence via RT wrote: Show quoted text
Thanks. Shit happens, you know? Best, David