Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

fix(concurrency): stop the transaction committer when the scheduler is done #1989

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

avi-starkware
Copy link
Collaborator

@avi-starkware avi-starkware commented Jun 17, 2024

This change is Reviewable

@avi-starkware avi-starkware changed the title fix(concurrency): stop the transaction committer when the scheduler is done. fix(concurrency): stop the transaction committer when the scheduler is done Jun 17, 2024
Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Cool, how did you find this?
:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @barak-b-starkware and @noaov1)

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.61%. Comparing base (d924c79) to head (a1b8d05).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1989   +/-   ##
=======================================
  Coverage   78.61%   78.61%           
=======================================
  Files          62       62           
  Lines        8895     8895           
  Branches     8895     8895           
=======================================
  Hits         6993     6993           
  Misses       1455     1455           
  Partials      447      447           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@avi-starkware
Copy link
Collaborator Author

@barak-b-starkware said his flow test got stuck in an infinite loop, so I tried to think of places where the scheduler might get stuck.
This is probably not the bug from Barak's flow test because his flow test does not use the method halt_scheduler, but it is still a bug.

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Then @OriStarkware must see this bug in his env. Please make sure

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @barak-b-starkware, @meship-starkware, @noaov1, and @OriStarkware)

@avi-starkware avi-starkware merged commit c7e4ee3 into main Jun 17, 2024
14 of 15 checks passed
@avi-starkware avi-starkware deleted the avi/concurrency/fix-halt-bug branch June 17, 2024 15:52
@barak-b-starkware
Copy link
Collaborator

crates/blockifier/src/concurrency/scheduler.rs line 31 at r1 (raw file):

            *self.commit_index_guard < self.scheduler.chunk_size,
            "The commit index must be less than the chunk size, since the scheduler is not done."
        );

Why do we need this assertion? It feels trivial/redundant. What do I miss here?

Code quote:

        assert!(
            *self.commit_index_guard < self.scheduler.chunk_size,
            "The commit index must be less than the chunk size, since the scheduler is not done."
        );

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants