-
Notifications
You must be signed in to change notification settings - Fork 99
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
Stop spawn_clean_orderpool_job on shutdown #349
Conversation
Benchmark results for
|
Date (UTC) | 2025-01-15T21:21:19+00:00 |
Commit | d3a2fca73b595d67ebbf9167396ebe29b532cf96 |
Base SHA | c1c5595d5bc08529375a38c50a2fc3f74e285e58 |
Significant changes
Benchmark | Mean | Status |
---|---|---|
gather_nodes_empty_account | -3.78% | Performance has improved. |
hashing_3000_elements | 5.07% | Performance has degraded. |
}; | ||
loop { | ||
tokio::select! { | ||
Some(header) = header_receiver.recv() => { |
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.
What happens is header_receiver.recv() gives None?
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.
Isn't this equivalent to what we had before? If it is not Some it will not perform that check and wait for the next iteration?
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 haven't tried it. Claude says None will not match and will not stop the loop so it may not behave exactly as before.
You get the exact behavior as before (stop con channel end) if you match header_res = header_receiver.recv() without the "Some" and then check match for Some (current code)/None (end)
@@ -334,33 +334,41 @@ where | |||
let handle = tokio::spawn(async move { |
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.
Usually this jobs need to be spawned as blocking and not as async tasks and its time consuming and blocks executor.
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.
Its probably fine here as this job happens between slots.
Sometimes, the Cargo.lock will differ between `BASE_SHA` and `HEAD_SHA`. This can cause the final checkout to `HEAD_SHA` to fail. This PR resolves that, by resetting any changes prior to the final checkout.
e27928c
to
d3a2fca
Compare
📝 Summary
Bug introduced in #327. Rbuilder would not stop with
Ctrl-C
because it would be waiting forspawn_clean_orderpool_job
to finish. But,spawn_clean_orderpool_job
would not handle the cancellation token.💡 Motivation and Context
✅ I have completed the following steps:
make lint
make test