Modify V3's control flow to use throw/catch

[errinlarsen]

Although the exception mechanism of raise and rescue is great for abandoning execution when things go wrong, it’s sometimes nice to be able to jump out of some deeply nested construct during normal processing. This is where catch and throw come in handy.

-- Programming Ruby 1.9 & 2.0: The Pragmatic Programmers' Guide, Thomas, Dave et al. (December, 2013), Pragmatic Bookshelf, p. 151

As I began to think about how to make this change, I discovered that removing the use of raise/rescue for control flow reduces the need to actually raise any Exceptions down to only one circumstance: when #fail! has been called from within #perform!. For this reason, I decided to flip the logic around #perform and #perform!.

• From: #perform always calling #perform!, and rescuing Failure
• To: #perform! always calling #perform, and raising Failure if failure?

I rewrote and reorganized a large number of specs in spec/support/lint.rb. First, the logical flipping of #perform/#perform! necessitated some changes. Second, I wanted to reduce the specs' dependence on the underlying mechanism of control flow (e.g. raise/rescue, throw/catch, etc.) and focus on the results of that control flow, instead. With that in mind, I'm testing for expected: order of before/run/after methods, expected success? or failure? value, whether or not the execution halts, and whether or not #rollback is called.

I thought the best place to throw a symbol was in the Interactor#succeed! and Interactor#fail! methods themselves. The symbol being thrown is, appropriately, :halt!, and the second parameter defines the reason for halting. This has the effect of returning that second parameter as the value of the catch, making it easy to check why we halted, and act accordingly.

I changed the #before/#after methods to #before_run/#after_run because I anticipate a few more "around" hooks in the future. For example, there is already a request for an #after_failure method. With that in mind, I wanted to be a bit more explicit with these method names.

Finally, I remove the Interactor::Success class as it is no longer needed.

[errinlarsen]

One of the advantages of using throw/catch, and implementing this by throwing a generic :halt! symbol, is that it will be easy to extend this later, if we need.

For example, someone may want a 3rd state introduced, like #interrupt!, or #timeout!.

This can easily be added now:

module Interactor
  def interrupt!(*args)
    context.interrupt!(*args)
    throw :halt!, :interrupted!
  end

  class Context < SimpleDelegator
    def interrupt!(context = {})
      @interrupted = true
      update(context)
    end
end

By throwing :halt!, we don't need to update any of the other methods when we add a new break-point condition.

In fact, developer's could easily throw :halt!, :some_custom_condition themselves if they wanted. That makes me wonder if we shouldn't update the context with the value of catch(:halt!) to facilitate that use.

module Interactor
  def perform
    halt_condition = catch(:halt!) do
      before_run
      run

      if catch(:halt!) { after_run } == :failure!
        rollback
        throw :halt!, :failure!
      end

      nil  # sets halt_condition to `nil' unless `:halt!' was thrown
    end

    context.update(halted: halt_condition) if halt_condition
  end
end

... or, it would probably be better to update the Context class to handle this explicitly so we don't interfere with end-developer's use of the Context's key namespace. We could create a #halted? predicate, a corresponding @halted ivar, and an accessor to get at the contents of @halted, which would default to nil and be populated with the value of the caught :halt! condition.

[trestrantham]

I'm really liking this approach thus far. Thank you for the contribution and thorough explanation of rationale. I'm going to pull this branch into a local project to better put it through the ringer.

As a personal note, I prefer before and after from an aesthetics perspective, but I can see the need for specific surrounds for run and failures. It may be overkill, but maybe we can have an outer surround of before and after and run-/failure-specific inner surrounds? Just a thought.

I do find the custom halt behavior intriguing and flexible though I haven't yet run into that specific use case (but could easily see it coming up in the future).

Great work!

[errinlarsen]

Thanks for the comments, @trestrantham!

I'm happy to change the #before/#after method names to whatever the consensus is; I have no dog in that fight, as they say! . I'll go ahead and change the #before_run/#after_run method names back to #before/#after when I fix a couple of other things (next paragraph). We can always re-name them in the future, if needed.

As well as that change, I also had a couple of small things I was going to "fix" in this branch, too; probably tomorrow morning. Specifically, in the specs, I needed a way to dynamically add methods to the instance being tested. I had a bit of brain mis-fire and dynamically included anonymous modules to achieve what I need, completely overlooking the obvious: def instance.before_run; succeed!; end;

After running those callback/hook methods around in my head for awhile, I thought it might be good to have a full-fledged callback sub-system mixed in, as well. That's something I think I'd enjoy writing. I'll play with it a bit and throw it in another branch and see what people think. I'll save any discussion (beyond the name of the two, specific #before/#after hooks) for that thread.

[avdi]

(@errinlarsen, thanks for inviting me to comment)

I'm always happy to see exceptions-for-control-flow replaced by throw/catch!

I have a mild, nitpicky aversion to symbols with bangs in them. I can't really justify this, other than to say that we use bangs in methods to differentiate a "surprising" method from a same-named unsurprising one, and since there isn't an "unsurprising" version of the symbols to compare to, I'd leave them off.

I haven't had time to look the code over in detail and fully grok it. That said, I'm slightly concerned about passing symbols as the payload of throw. A symbol doesn't convey much. Especially in the case of a failure, that suggests to me that important information might be being thrown away at the site of the failure. I'd suggest that you find ways to represent the different outcomes as objects.

Which might be as simple as representing failure information as exceptions... in which case, it might make more sense to use raise an rescue exceptions in the failure case, and only throw :halt for non-failure early terminations. In moving away from exceptions-as-control-flow, it's important not to go too far in the other direction and use control-flow-for-exceptions.

[errinlarsen]

... nitpicky aversion to symbols with bangs in them. ... I'd leave them off.

My thinking, in regards to adding those bangs to the symbols being returned, was concern for the end-developers' use of the Context, and specifically the keys being added/used. I am anticipating that, in the future, Interactor will be adding the payload (i.e. these :halt! symbols) to the Context. I thought that, like you, most are averse to using bangs in their symbols, and by using the bangs I'd be less likely to stomp on anyone else's keys already in use by the Context.

However, that is a lot of "ifs" and "might-bes"! If this library automatically adds the payload of throw(:halt!, some_payload) to the Context, and if that payload is added via a key/value pair in the Context's Hash, there might be a conflict with a developer's previous or subsequent use of that same key for some other purpose.

I think I agree with you and I believe the "You Aren't Gonna Need It" practice applies to my points, above.

... I'm slightly concerned about passing symbols as the payload of raise. ... it's important not to go too far in the other direction and use control-flow-for-exceptions.

I agree with both concerns. I was originally uncomfortable with passing any payload in the throw(:halt) calls (I assume you meant throw in your comment about passing symbols as the payload). I added the payload only because I needed to handle catch(:halt) differently if they were thrown during the #after method. I believe this scenario is better handled by Exceptions and a begin/rescue/end block.

I'll make some modifications.

[avdi]

Er, yes. I meant throw. Edited :smile:

[errinlarsen]

I implemented some suggestions (thanks, @avdi) and pushed.

---

raise Failure, "fail!: called in #{self.class}##{caller[0][/`([^']*)'/, 1]}"

The above fanciness was added to #fail! and will result in a (hopefully) more meaningful exception message. Now, this interactor:

class DoStuff
  include Interactor
  def before
    fail!(message: "Can't do stuff.")
  end
end

raises this exception:

[3] pry(main)> DoStuff.perform!
Interactor::Failure: fail! called in DoStuff#before
from /some/path/interactor/lib/interactor.rb:74:in `fail!'

[avdi]

This is just an FYI, not necessarily a reason for change: #fail is an existing method on Kernel. It is an alias of #raise. I note this only because not everyone knows this these days, and if someone decided to rename #fail!to #fail at some point, it could theoretically break things.

[gregory]

:+1:

[laserlemon]

Closing in favor of v3 for the moment. Version 3.0.0 will most likely ship without the concept of "hard success" that halts execution.