feat: /install/tasks support singleflight#675
feat: /install/tasks support singleflight#675fatelei wants to merge 1 commit intolanggenius:mainfrom
Conversation
dosubot
bot
added
size:L
![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 introduces request deduplication using the singleflight package for fetching plugin installation tasks, aimed at reducing redundant database queries. It also adds unit tests utilizing an in-memory SQLite database to verify the deduplication logic. The review feedback highlights critical concurrency concerns, specifically the risk of data races when returning shared pointers or slices to multiple concurrent callers, and suggests consolidating the separate singleflight groups into a single instance for better code organization.
| v, err, _ := installationTasksGroup.Do(key, func() (interface{}, error) { | ||
| tasks, err := db.GetAll[models.InstallTask]( | ||
| db.Equal("tenant_id", tenant_id), | ||
| db.OrderBy("created_at", true), | ||
| db.Page(page, page_size), | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return tasks, nil | ||
| }) |
There was a problem hiding this comment.
The use of singleflight with reference types (like slices or pointers) can lead to data races. singleflight.Do returns the exact same object to all concurrent callers. If any caller or subsequent logic (e.g., middleware or decorators) modifies the returned slice or its elements before serialization, it will affect all other concurrent requests and cause a data race. Consider returning a deep copy of the result or ensuring the returned objects are treated as strictly immutable.
| v, err, _ := installationTaskGroup.Do(key, func() (interface{}, error) { | ||
| task, err := db.GetOne[models.InstallTask]( | ||
| db.Equal("id", task_id), | ||
| db.Equal("tenant_id", tenant_id), | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return task, nil | ||
| }) |
There was a problem hiding this comment.
Similar to the list fetch, returning a pointer (*models.InstallTask) via singleflight is risky. All concurrent callers receive the same pointer. If one caller modifies the task object (e.g., updating a status field or modifying the Plugins slice), the change is visible to all other callers, leading to potential data races and inconsistent state. It is safer to return a deep copy of the task to each caller.
| var ( | ||
| installationTasksGroup singleflight.Group | ||
| installationTaskGroup singleflight.Group | ||
| ) |
There was a problem hiding this comment.
Using multiple singleflight.Group instances within the same service is redundant when the keys are already prefixed with distinct identifiers (e.g., tasks: and task:). A single singleflight.Group can handle deduplication for all keys in this service, which simplifies the code and reduces global state.
var (
installationGroup singleflight.Group
)1 participant
Description
fix #674
Type of Change
Essential Checklist
Testing
Bug Fix (if applicable)
Fixes #123orCloses #123)Additional Information
Please provide any additional context that would help reviewers understand the changes.