Skip to content
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

Race condition issue #467

Closed
nhaancs opened this issue Mar 13, 2025 · 3 comments
Closed

Race condition issue #467

nhaancs opened this issue Mar 13, 2025 · 3 comments

Comments

@nhaancs
Copy link
Contributor

nhaancs commented Mar 13, 2025

func (b *Business) Update(ctx context.Context, prd Product, up UpdateProduct) (Product, error) {

I think the Update method has a potential race condition because it performs the update operation in two separate steps: query and update. This creates a potential time window between reading and writing where another transaction could modify the same product.

@ameghdadian
Copy link
Contributor

Both operations(query and update) are performed in a single transaction:

func BeginCommitRollback(log *logger.Logger, bgn sqldb.Beginner) web.MidFunc {

Considering a transaction has ACID properties, two transactions are not likely to affect each other(Isolation property). So I think there should be no problem.

@nhaancs
Copy link
Contributor Author

nhaancs commented Mar 13, 2025

Both operations(query and update) are performed in a single transaction:

service/app/sdk/mid/transaction.go

Line 16 in c3a19bf

func BeginCommitRollback(log *logger.Logger, bgn sqldb.Beginner) web.MidFunc {
Considering a transaction has ACID properties, two transactions are not likely to affect each other(Isolation property). So I think there should be no problem.

Hi @ameghdadian, I dont see the project use the transaction middleware for product update route.

app.HandlerFunc(http.MethodPut, version, "/products/{product_id}", api.update, authen, ruleAuthorizeProduct)

Even they are executed in a transaction, other transactions can possibly read the the old value of the product if you haven't set the right isolation level in the database settings.

@ardan-bkennedy
Copy link
Contributor

Updates are what udpates are. To minimize locks you need to just accept the last update in sets the state of the Database. That is why you need to pull a fresh version of the database's state before you do the update. You must make sure what you are updating is valid state.

I've worked on patient systems where they would "LOCK" a patient record at the application level and would not allow more than one person to get that lock. Sounds good right? NO. Now that person with the LOCK goes to lunch, walks away for hours, and no one else can service that patient record.

How we implemented the update is valid for this system.

But for sure someone could have started an edit 4 days ago, have several updates happen in front of them, then they can apply their update and the state of the DB would be that last update.

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

No branches or pull requests

3 participants