-
Notifications
You must be signed in to change notification settings - Fork 777
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: prevent batch infinite loop with arg length #1794
fix: prevent batch infinite loop with arg length #1794
Conversation
🤔 This is the test failure --
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking over the generated code, I think there's a cleaner way to implement the callbacks without having the check the return value of the error.
func (b *UpdateValuesBatchResults) Exec(f func(int, error)) {
defer b.br.Close() // Close is safe to call multiple times
for i := 0; i < b.tot; i++ {
if b.closed {
f(i, errrors.New("batch already closed"))
continue
}
_, err := b.br.Exec()
if f != nil {
f(i, err)
}
}
}
func (b *UpdateValuesBatchResults) Close() error {
return b.closed = true
return b.br.Close()
}
Thoughts?
Oh yeah that's super clean, thanks! I'll circle back around and request another review after I've updated everything. |
dc8a552
to
d262908
Compare
d262908
to
7c6ac10
Compare
|
Because the batch queries are only being closed when checking for 2 errors (batch results closed or no result), other errors cause the generated code to infinite loop. This changes from a for loop that only breaks on checking the errors to a for loop that also considers the the total number of params passed in.
Some errors will not infinite loop if you close the results yourself. However, there are other errors that you can't close --
Bad queries typically happen in tests, but the infinite loop has eaten up >20Gb of RAM within a few minutes for me before.
This is usually what happens. It infinite loops, so I can't interrupt the process, and then I have to do
ps aux | grep <>
to find and kill the process manually:Fixes #1678 (which was closed by the reporter since they found a workaround)