Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

5688: Run callbacks for children within fibers #5837

Merged
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
0b103f7
Run callbacks for children within fibers
adviti-mishra Jul 3, 2024
5e69a45
Comments for run child callbacks with around fibers
adviti-mishra Jul 16, 2024
1092cf0
Removed the recursive implementation and renamed the fiber implementa…
adviti-mishra Jul 16, 2024
8933938
Message, summary, and resolution of invalid around callback implement…
adviti-mishra Jul 16, 2024
083d563
Class for invalid around callback error
adviti-mishra Jul 16, 2024
c84a182
try catch block to raise invalid around callbacks error when it is de…
adviti-mishra Jul 16, 2024
de260db
Require the invalid around callback error
adviti-mishra Jul 16, 2024
dd7c35b
Merge branch 'master' into 5688-fibers-run-child-callbacks
adviti-mishra Jul 17, 2024
005c5b4
Refactored the rescuing of the exception to make it more elegant
adviti-mishra Jul 17, 2024
877f41c
invalid around callback error file is linter-approved
adviti-mishra Jul 17, 2024
16c9c00
Fixed indentation for the rescue block
adviti-mishra Jul 17, 2024
1732a2c
Spec for the case a user incorrectly defines an around callback witho…
adviti-mishra Jul 18, 2024
f2ea1d5
raising an invalid around callback error earlier on as soon as a fibe…
adviti-mishra Jul 18, 2024
85fb2d5
Merge branch 'master' into 5688-fibers-run-child-callbacks
adviti-mishra Jul 18, 2024
76f69f9
Changed the names of the classes for the invalid around callback spec
adviti-mishra Jul 19, 2024
137dbe5
Modified the class names for the around callback without yield to avo…
adviti-mishra Jul 19, 2024
215513d
Made the creation fo embedded documents in the around callbacks witho…
adviti-mishra Jul 19, 2024
8ea5046
Update spec/mongoid/interceptable_spec.rb to use logger
adviti-mishra Jul 19, 2024
49552d3
removed the temporary field
adviti-mishra Jul 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ en:
A collation option is only supported if the query is executed on a MongoDB server
with version >= 3.4."
resolution: "Remove the collation option from the query."
invalid_around_callback:
message: "An around callback must contain a yield in its definition."
summary: "The block needs to be yielded to for around callbacks to function as intended."
resolution: "Ensure there is a yield statement inside the body of the around callback."
invalid_async_query_executor:
message: "Invalid async_query_executor option: %{executor}."
summary: "A invalid async query executor was specified.
Expand Down
1 change: 1 addition & 0 deletions lib/mongoid/errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
require "mongoid/errors/immutable_attribute"
require "mongoid/errors/in_memory_collation_not_supported"
require "mongoid/errors/invalid_auto_encryption_configuration"
require "mongoid/errors/invalid_around_callback"
require "mongoid/errors/invalid_async_query_executor"
require "mongoid/errors/invalid_collection"
require "mongoid/errors/invalid_config_file"
Expand Down
16 changes: 16 additions & 0 deletions lib/mongoid/errors/invalid_around_callback.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# frozen_string_literal: true

module Mongoid
module Errors
# This error is raised when an around callback is
# defined by the user without a yield
class InvalidAroundCallback < MongoidError
Copy link
Contributor

@johnnyshields johnnyshields Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@johnnyshields johnnyshields Jul 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@johnnyshields johnnyshields Jul 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Create the new error.
#
# @api private
def initialize
super(compose_message('invalid_around_callback'))
end
end
end
end
31 changes: 18 additions & 13 deletions lib/mongoid/interceptable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -161,29 +161,34 @@ def _mongoid_run_child_callbacks(kind, children: nil, &block)
# Execute the callbacks of given kind for embedded documents including
# around callbacks.
#
# @note This method is prone to stack overflow errors if the document
# has a large number of embedded documents. It is recommended to avoid
# using around callbacks for embedded documents until a proper solution
# is implemented.
#
# @param [ Symbol ] kind The type of callback to execute.
# @param [ Array<Document> ] children Children to execute callbacks on. If
# nil, callbacks will be executed on all cascadable children of
# the document.
#
# @api private
def _mongoid_run_child_callbacks_with_around(kind, children: nil, &block)
child, *tail = (children || cascadable_children(kind))
children = (children || cascadable_children(kind))
with_children = !Mongoid::Config.prevent_multiple_calls_of_embedded_callbacks
if child.nil?
block&.call
elsif tail.empty?
child.run_callbacks(child_callback_type(kind, child), with_children: with_children, &block)
else
child.run_callbacks(child_callback_type(kind, child), with_children: with_children) do
_mongoid_run_child_callbacks_with_around(kind, children: tail, &block)

return block&.call if children.empty?

fibers = children.map do |child|
Fiber.new do
child.run_callbacks(child_callback_type(kind, child), with_children: with_children) do
Fiber.yield
end
end
end

fibers.each(&:resume)

block&.call

fibers.reverse.each(&:resume)

rescue FiberError
raise Mongoid::Errors::InvalidAroundCallback
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@alexbevi alexbevi Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@adviti-mishra adviti-mishra Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@johnnyshields johnnyshields Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

end

# Execute the callbacks of given kind for embedded documents without
Expand Down
Loading