-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
adviti-mishra
merged 19 commits into
mongodb:master
from
adviti-mishra:5688-fibers-run-child-callbacks
Jul 19, 2024
Merged
Changes from all 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 5e69a45
Comments for run child callbacks with around fibers
adviti-mishra 1092cf0
Removed the recursive implementation and renamed the fiber implementa…
adviti-mishra 8933938
Message, summary, and resolution of invalid around callback implement…
adviti-mishra 083d563
Class for invalid around callback error
adviti-mishra c84a182
try catch block to raise invalid around callbacks error when it is de…
adviti-mishra de260db
Require the invalid around callback error
adviti-mishra dd7c35b
Merge branch 'master' into 5688-fibers-run-child-callbacks
adviti-mishra 005c5b4
Refactored the rescuing of the exception to make it more elegant
adviti-mishra 877f41c
invalid around callback error file is linter-approved
adviti-mishra 16c9c00
Fixed indentation for the rescue block
adviti-mishra 1732a2c
Spec for the case a user incorrectly defines an around callback witho…
adviti-mishra f2ea1d5
raising an invalid around callback error earlier on as soon as a fibe…
adviti-mishra 85fb2d5
Merge branch 'master' into 5688-fibers-run-child-callbacks
adviti-mishra 76f69f9
Changed the names of the classes for the invalid around callback spec
adviti-mishra 137dbe5
Modified the class names for the around callback without yield to avo…
adviti-mishra 215513d
Made the creation fo embedded documents in the around callbacks witho…
adviti-mishra 8ea5046
Update spec/mongoid/interceptable_spec.rb to use logger
adviti-mishra 49552d3
removed the temporary field
adviti-mishra File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
# Create the new error. | ||
# | ||
# @api private | ||
def initialize | ||
super(compose_message('invalid_around_callback')) | ||
end | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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:
everywhere in my app.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.