5688: Run callbacks for children within fibers

[adviti-mishra]

Using Ruby Fibers to run callbacks for children. This helps avoid StackOverflow when callbacks cascade

[johnnyshields]

@adviti-mishra cool idea!

[jamis]

This is looking so great! Wonderful work on this. 🎉

[jamis]

The rubocop:todo all line is a bandaid for existing files, so that we can fix linter errors on a file-by-file basis as we work in them. For new files, let's leave that line out and just make them linter-approved from the start. Note that a lot of our existing files have linter issues, so you'll almost certainly see the linter complain about this file, at first. Fortunately, it's a small file. :)

(You can run the linter locally on a single file, with rubocop path/to/file, or on the whole project with just rubocop.)

Suggested change

# rubocop:todo all

[adviti-mishra]

Thank you for the context. Pushed this change because the file is linter-approved now!

[jamis]

This isn't wrong, but it can be made more concise by moving the rescue to the level of the method itself.

The refactoring goes like this. Given this implementation:

def fn(list)
  list.each do |item|
    begin
      item.do_something
    rescue Exception
      raise OtherException
    end
  end
end

You can refactor it by extracting the begin/rescue/end to the level of the method:

def fn(list)
  begin
    list.each(&:do_something)
  rescue Exception
    raise OtherException
  end
end

However, every method is implicitly a begin/end block, so the begin and end are redundant. You can remove them, like so:

def fn(list)
  list.each(&:do_something)
rescue Exception
  raise OtherException
end

[adviti-mishra]

This is great feedback. Thank you! Pushed these changes

[jamis]

Great work! 🚀

[comandeo-mongo]

Great job @adviti-mishra, such an elegant solution! 👍

[comandeo-mongo]

Are we sure that any FiberError is caused by non providing yield in a callback? If no, can we somehow detect what happens or adjust the error description?

[alexbevi]

From the API docs a FiberError is "Raised when an invalid operation is attempted on a Fiber, in particular when attempting to call/resume a dead fiber, attempting to yield from the root fiber, or calling a fiber across threads."

Not sure if differentiating these failure conditions would help any, but error specificity is never a bad thing.

[adviti-mishra]

Since our program and not the user handles what fibers are created and resumed, the only condition under which a dead fiber seems to be resumed is if the user does not call yield. This falls under the "attempting to call/resume a dead fiber" bucket. I believe specificity here may be helpful because users don't even need to know fibers are used underneath. However, I'm fine with leaving the error message to be more general as well.

[johnnyshields]

I would think suppressing these fiber errors (possibly with a log line only) would be the right way to go. Probably these will mostly pop-up when terminating an app.

[jamis]

As I think about this, it occurs to me that there could be situations where a user might do something with fibers in their own code, in a callback (weird, but feasible). FiberError exceptions from their code could then be potentially captured here, resulting in a confusing error message.

I wonder if it might be safer to explicitly look to see if the fiber is still #alive? after the first resume, and if it isn't, raise the InvalidAroundCallback exception there?

f = Fiber.new { nil }
f.alive? #-> true
f.resume
f.alive? #-> false

[johnnyshields]

@jamis I think raising InvalidAroundCallback here if the fiber dies prematurely may not be a good idea. It's probably best to let the Fiber die gracefully with a log line. The scenario you are describing is analogous to a non-Fiber around callback not calling yield but instead exiting early. This is not encouraged but it is valid behavior currently, I believe.

TLDR; I do not think we should make Fibers do anything that current callbacks don't do.

[jamis]

Looking good. Just one suggestion for the specs. 👍

[jamis]

If you declare a value at the context level like this, it will be evaluated once for the entire context. In this case, it doesn't matter (because there's only one test), but in general you want the value to be "clean" for each test. To accomplish this, rspec provides a let method to declare these variables in a way that allows you to re-evaluate them for each test, e.g.:

Suggested change

	foo = Foo.create
	2.times { foo.bars.build }
	let(:foo) do
	Foo.create.tap do |foo|
	2.times { foo.bars.build }
	end
	end

We can make it a bit more concise by providing the bars list to the #create call, like this:

Suggested change

	foo = Foo.create
	2.times { foo.bars.build }
	let(:foo) { Foo.create(bars: [ Bar.new, Bar.new ]) }

[johnnyshields]

As per comment elsewhere, I am strongly opposed to introducing a new runtime error during persistence flow which users will need to handle in their code. Callbacks today do not raise such errors.

Now I will have to do:

begin
  dog.save!
rescue InvalidAroundCallback
  # ...
end

everywhere in my app.

[jamis]

@johnnyshields -- this exception is raised only for a single case: when an around callback fails to yield. This will always indicate a bug in the users code. If we did not raise the exception here, it would manifest instead as a FiberError, which would be confusing to users because it does not describe what the issue is.

Rather than risk confusing users with a seeming non-sequitur, we're opting to raise this new error to help direct them to where the bug is in their code. If you always yield from your around callbacks, you'll never encounter the error.

[johnnyshields]

@jamis I understand that, but today (prior to the Fiber version) you can do an around callback which doesn't yield and it doesn't raise any error as far as I'm aware--it just doesn't continue the callback chain.

Is there a possibility that users are using non-yielding callbacks for flow control of their actions?

Also, what if we throw :abort in a callback?

I think this should be looked at in more detail...

[jamis]

Thanks for your concern about this issue, @johnnyshields, but we really have investigated this and and have looked at this, in quite a bit of detail.

An around callback that fails to yield is not as innocuous as you think. Not only does it just fail to "continue the callback chain" -- it fails to perform the intended operation at all. That is to say, if you have an around_save without a yield, and you save the record, it will silently fail to save the record at all.

This is why I said that this is a bug in their code, and I don't feel bad about surfacing this noisily now, where we didn't before. If someone's code "breaks" because of this, it will break in a way that it should have broken before. I do not believe there is any way that anyone was using non-yielding around callbacks for any useful purpose, because they wouldn't have been functional at all.

[johnnyshields]

Yes, what if someone is intentionally using that behavior like a validation in order to abort the save flow? For example:

around_save :do_not_save_if_red

def do_not_save_if_red
  if color == "red"
    # report error somehow
  else
    yield
  end
end

App owners will now need to check that no callback is implementing the above pattern. Callbacks in Mongoid models now work differently than controllers, for example, which would allow the above.

[jamis]

Looks good to me! 👍 🎉