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

rollback on panic #119

Merged
merged 2 commits into from Dec 9, 2021
Merged

rollback on panic #119

merged 2 commits into from Dec 9, 2021

Conversation

hahashraf
Copy link
Contributor

@hahashraf hahashraf commented Oct 29, 2021

Fixes #76

I tried to do the minimum amount of changes to fix the issue.
UPDATE: did a very small amount of clean up to help me understand a bit better.

There are currently no tests around this. I explored the current tests, and after a while (lots of abstraction), I was able to understand them for the most part. However, it seems that currently (and please correct me if I'm wrong) there are no existing tests actually testing the existing defer statement? I would have expected at least some testing of a "non-retryable" error resulting in a rollback, but either there are not any or I couldn't find them.

If someone could advise me on what tests for rollback (whether or not via panic or "non-retryable" error, probably should add tests for both) should look like, I'd much appreciate it and am happy to work on them.

Let me know if you have questions, thank you! 🙏

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@@ -3,7 +3,7 @@ module github.com/cockroachdb/cockroach-go/v2
go 1.13

require (
github.com/gofrs/flock v0.8.1 // indirect
github.com/gofrs/flock v0.8.1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just from go mod download; go mod tidy on my machine, happy to revert if I got it wrong.

Copy link
Contributor

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this change! it looks good overall -- can you adjust the comment and rebase this?

crdb/common.go Outdated
}
// We got an error; let's see if it's a retryable one and, if so, restart.

// We got an error (i.e. err != nil) if at this point
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this comment needs to be fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure what you mean, but put it back to exactly what was there. 👍

@hahashraf hahashraf requested a review from rafiss December 9, 2021 17:56
@hahashraf
Copy link
Contributor Author

hahashraf commented Dec 9, 2021

@rafiss Thanks for taking a look.

Rebased against master, fixed the comment, and actually reverted a few changes, so the change set is now smaller than before (had a misunderstanding of Go named return values).

Just to re-iterate: still no testing around these changes, please advise if you'd like me to add some and if you have any ideas/thoughts on how to accomplish that. Totally your call on adding or merging as is.

Thanks again! 🙏

Copy link
Contributor

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this lgtm! i wouldn't worry too much about testing this

@rafiss rafiss merged commit 561007c into cockroachdb:master Dec 9, 2021
@rafiss
Copy link
Contributor

rafiss commented Dec 9, 2021

If you do want to keep working on the tests for this, here's what you can try:

  • implement your own (testing only) version of the crdb.Tx interface that can detect when Commit and Rollback have been called.
  • call crdb.ExecuteInTx with your custom Tx and a function that panics. (and another test for a function that returns an error.
  • make sure Commit has not been called, and that Rollback has been called.

@hahashraf hahashraf deleted the fix/rollback-on-panic branch December 9, 2021 19:34
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.

Prevent transaction from committing on panic.
3 participants