search

DATA-DOG / go-txdb

Immutable transaction isolated sql driver for golang
Other
667 stars 48 forks source link

fix beginTxOnce() context cancel #64

Closed daisuke0925m closed 5 months ago

daisuke0925m commented 8 months ago

we received this error.

transaction has already been committed or rolled back errors

fix: https://github.com/DATA-DOG/go-txdb/issues/43#issuecomment-1625076132

flimzy commented 7 months ago

I'm investigating this closer today, and trying to reproduce it locally, to verify that this is the optimal solution, bit I'm not able to trigger the error, using either your test, or using my own techniques.

Can you provide any additional insight into the circumstances under which this problem surfaces?

Also, which database driver are you using?

daisuke0925m commented 7 months ago

@flimzy

I can reproduce this by running TestShouldRunWithHeavyWork locally without modifying beginTxOnce() in db_go18.go. https://github.com/DATA-DOG/go-txdb/pull/64/files#diff-b0b479cb93a7eee6d8905ffc4e94df874afad18b4a97edbf3e2a1402a20db634R57-R63

Also, I am using mysql in my actual development environment.

Warashi commented 7 months ago

@flimzy @daisuke0925m I also ran the test. I could not reproduce failure with the original test that @daisuke0925m wrote, but successfully reproduced after modifying 10000 to 1000000 in this line. https://github.com/DATA-DOG/go-txdb/pull/64/files#diff-842765a20a0a937a8eb5267b53fb11b7493cc6c2a6a8871e1da2b684702c5bc9R841

iamnoah commented 6 months ago

We have a test suite that was plagued by this error in GHA and this change seems to fix it.

I also never was able to reproduce it on my laptop, you need a machine a sluggish as a CI runner to get there apparently.

flimzy commented 6 months ago

I'm on holiday now, but when I return, I intend to get back into this and merge it, or an alternative fix, based on my investigation.

iamnoah commented 6 months ago

Just wanted to bump. Been using this for 3 weeks now and haven't seen the issue resurface or any other problems.

flimzy commented 5 months ago

My apologies to everyone for my delay in getting back to this. I was able to re-produce the problem locally with the hints above. And while I don't think the underlying structure of how go-txdb handles contexts is ideal (I think I've spotted at least one other possible race condition), this does appear to be an improvement in behavior at least, so worth merging. I may try to do additional refactoring later.