-
Notifications
You must be signed in to change notification settings - Fork 110
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
fix(996): Do not release database lock early for subscriptions #997
Conversation
a8d1ae2
to
c79318e
Compare
b422b08
to
690239b
Compare
c79318e
to
48476ce
Compare
@@ -85,11 +85,9 @@ impl ModuleSubscriptions { | |||
// This also makes it possible for `broadcast_event` to get scheduled before the subsequent part here | |||
// but that should not pose an issue. | |||
let mut subscriptions = self.subscriptions.write(); | |||
drop(tx); |
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.
I feel we do not need to drop(tx)
early for any correctness issue, we can do it being more conserative.
dropping subscription lock should be enough as that will also not let the reducers to commit (reducer need subscription read lock to commit), so their eval_incr
will also not start.
if we not drop(tx)
early, reducer execution will not happen parallel to eval
, though right now we are letting execution to happen but not commit (which is also fine).
At the end, it depends whether we want to be more conservative or look for bit more perfomance.
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.
We discussed async: Holding the tx for the lifetime of the call does keep a reducer from executing until we push onto the send queue, but that shouldn't have a noticeable impact on throughput, plus it makes testing easier. So I think we're both in favor of this from a correctness standpoint.
690239b
to
ccfd964
Compare
48476ce
to
f2cc2fd
Compare
f2cc2fd
to
d2d5782
Compare
Fixes #996.
Description of Changes
Please describe your change, mention any related tickets, and so on here.
API and ABI breaking changes
If this is an API or ABI breaking change, please apply the
corresponding GitHub label.
Expected complexity level and risk
How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.
This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.
If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.
Testing
Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!