Do-notation: wrapping method with global rescue works incorrect

[igor-alexandrov]

Do-notation in method that has a global rescue works incorrectly. Here is an example.

class Documents::CreateDocument
  include Dry::Monads::Result::Mixin
  include Dry::Monads::Do.for(:call)

  def call(document, params)
    params = yield validate(params)
    document = yield persist(document, params)
    document = yield GlobalContainer['document.services.rename_document_command'].call(document)
    document = yield GlobalContainer['document.services.create_affiliations'].call(document)

    Success(document)
  rescue => e
    GlobalContainer['support.services.exception_notifier'].call(e)
    Failure(e)
  end

  # ...
end

Lets suggest that validate is a method that returns Dry::Monads::Failure for incorrect input.

[1] pry(main)> incorrect_params = {category_id: 0}
=> {:category_id=>0}
[2] pry(main)> result = Documents::CreateDocument.new.call(Document.new, incorrect_params)
=> Failure(#<Dry::Monads::Do::Halt: Dry::Monads::Do::Halt>)

I expected to receive Failure with result of #validate method, instead of it I received Failure with Dry::Monads::Do::Halt. This happens because https://github.com/dry-rb/dry-monads/blob/master/lib/dry/monads/do.rb#L137 is not called when Halt is raised here https://github.com/dry-rb/dry-monads/blob/master/lib/dry/monads/do.rb#L132. Halt is captured by global rescue is original method.

From my point of view this can be fixed by aliasing and calling original method instead of overriding it.

[flash-gordon]

Basically, wildcard rescue is an anti-pattern so it's no surprise you run into issues with it. raise/rescue is an implementation detail of do notation and you're not supposed to catch exceptions by design. Monads are a way to avoid exceptions, in my opinion, mixing them together is not a good idea. I'd catch exceptions on a higher level. That said, this particular issue is fixable and I'll do it before pushing a new release.

[igor-alexandrov]

I understand that it is an anti-pattern example. In my example wildcard rescue is used in each service object to call notification service, this logic can be moved to controller, but I like it more in services.

[flash-gordon]

Ugh, nope, not gonna work. People may depend on the current behavior in situations like this

def call
  transaction do
    yield op1
    yield op2
  end
end

where transaction looks like this

def transaction
  begin_transaction
  yield 
  commit_transaction
rescue => e
  roll_back_transaction
  raise e
end

What I had in mind is changing Halt < StandardError to Halt < Exception but this would break the transaction's logic. I'm pretty sure ActiveRecord and Sequel handle this in the correct form, i.e. rescue Exception => e but I'm not certain about user code so it's going to stay this way. BTW, aliasing methods won't make a difference here.

[flash-gordon]

If you want to have this behavior across all your objects you may want to consider wrapping them on registration within a container, i.e. every object can be wrapped with a proc:

wrapped = proc do |*args|
  obj.(*args)
rescue => e
  # ...
  Failure(e)
end

[igor-alexandrov]

Ok, got it.

Another approach is to rescue Halt and then global rescue:

class Documents::CreateDocument
  def call
    # ...
  rescue Dry::Monads::Do::Halt => e
    e.result
  rescue => e
    GlobalContainer['support.services.exception_notifier'].call(e)
    Failure(e)
  end
end