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

fix beginTxOnce() context cancel #64

Merged
merged 2 commits into from
May 23, 2024

Conversation

daisuke0925m
Copy link
Contributor

@daisuke0925m daisuke0925m commented Mar 18, 2024

we received this error.

transaction has already been committed or rolled back errors

fix: #43 (comment)

@daisuke0925m daisuke0925m force-pushed the fix/TestShouldRunWithHeavyWork branch from ffb24e3 to d688654 Compare March 18, 2024 09:10
@daisuke0925m daisuke0925m force-pushed the fix/TestShouldRunWithHeavyWork branch from d688654 to 08031bc Compare March 18, 2024 09:16
@daisuke0925m daisuke0925m marked this pull request as ready for review March 18, 2024 09:17
@daisuke0925m daisuke0925m changed the title add TestShouldRunWithHeavyWork fix beginTxOnce() context cancel Mar 18, 2024
db_go18.go Outdated Show resolved Hide resolved
@flimzy
Copy link
Collaborator

flimzy commented Mar 27, 2024

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
Copy link
Contributor Author

@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
Copy link

Warashi commented Mar 27, 2024

@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
Copy link

iamnoah commented Apr 26, 2024

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
Copy link
Collaborator

flimzy commented Apr 29, 2024

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
Copy link

iamnoah commented May 20, 2024

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

@flimzy
Copy link
Collaborator

flimzy commented May 23, 2024

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.

@flimzy flimzy merged commit 367bc92 into DATA-DOG:master May 23, 2024
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

Successfully merging this pull request may close these issues.

Occasional sql: transaction has already been committed or rolled back errors
4 participants