refactor(block): improve cancellation #2741
Conversation
Summary of ChangesHello @julienrbrt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the block execution logic to enhance cancellation robustness. It temporarily modifies the context used for updating block height and state, switching from the executor's context to Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Claude finished @julienrbrt's task —— View job Code ReviewI've reviewed this PR that improves cancellation handling in the block executor. Here's my comprehensive analysis: SummaryThis PR switches from Review Findings✅ Positive Aspects
|
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a potential state inconsistency issue by ensuring that state and height updates are not cancelled after a block has been saved. This is achieved by using context.TODO() for these critical operations. My review suggests a small improvement to use context.Background() instead, as it's more idiomatic for this use case. I've also proposed an improved comment to clarify the reasoning for this change.
| // Once the SaveBlockData has been saved we must update the height and the state. | ||
| // context.TODO() should be reverted to the real context (e.ctx) once https://github.com/evstack/ev-node/issues/2274 has been implemented, this prevents context cancellation | ||
| if err := e.store.SetHeight(context.TODO(), newHeight); err != nil { | ||
| return fmt.Errorf("failed to update store height: %w", err) | ||
| } | ||
|
|
||
| if err := e.updateState(e.ctx, newState); err != nil { | ||
| if err := e.updateState(context.TODO(), newState); err != nil { | ||
| return fmt.Errorf("failed to update state: %w", err) | ||
| } |
There was a problem hiding this comment.
While context.TODO() works, context.Background() is more idiomatic for creating a non-cancellable context. It more clearly communicates the intent to run a critical operation that must not be cancelled.
I've updated the code to use context.Background() and improved the comment to clarify the rationale.
| // Once the SaveBlockData has been saved we must update the height and the state. | |
| // context.TODO() should be reverted to the real context (e.ctx) once https://github.com/evstack/ev-node/issues/2274 has been implemented, this prevents context cancellation | |
| if err := e.store.SetHeight(context.TODO(), newHeight); err != nil { | |
| return fmt.Errorf("failed to update store height: %w", err) | |
| } | |
| if err := e.updateState(e.ctx, newState); err != nil { | |
| if err := e.updateState(context.TODO(), newState); err != nil { | |
| return fmt.Errorf("failed to update state: %w", err) | |
| } | |
| // Once block data is saved, height and state must be updated even if the context is cancelled. | |
| // Using context.Background() prevents inconsistent state. See issue #2274. | |
| if err := e.store.SetHeight(context.Background(), newHeight); err != nil { | |
| return fmt.Errorf("failed to update store height: %w", err) | |
| } | |
| if err := e.updateState(context.Background(), newState); err != nil { | |
| return fmt.Errorf("failed to update state: %w", err) | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2741 +/- ##
=======================================
Coverage 62.17% 62.17%
=======================================
Files 79 79
Lines 8497 8497
=======================================
Hits 5283 5283
Misses 2721 2721
Partials 493 493
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* main: feat(store)!: add batching for atomicity (#2746) refactor(apps): rollback cmd updates (#2744) chore: add makefile for tools (#2743) chore: fix markdown lint (#2742) build(deps): Bump the all-go group across 5 directories with 6 updates (#2738) refactor(block): improve cancellation (#2741) chore: make the prompt go oriented (#2739) perf(block): use `sync/atomic` instead of mutexes (#2735)
Overview
Once the SaveBlockData has been saved we must update the height and the state.
context.TODO() should be reverted to the real context (e.ctx) once #2274 has been implemented, this prevents context cancellation messing up state.