Ensure bufferWriter is always closed in Migration.Buffer and propagate close errors #1308
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@dhui could you pls check this small PR |
dhui
reviewed
Aug 19, 2025
migration.go
Outdated
| // Always close bufferWriter, even on error, to prevent deadlocks. | ||
| // This lets Buffer know that there is no more data coming. | ||
| defer func() { | ||
| if err := m.bufferWriter.Close(); err != nil && bufferWriterCloseErr == nil { |
There was a problem hiding this comment.
Use a named return and errors.Join(). See this example
migration.go
Outdated
| if err := m.bufferWriter.Close(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // it's safe to close the Body too | ||
| if err := m.Body.Close(); err != nil { |
There was a problem hiding this comment.
should also defer this Close() and preserve the close ordering
|
@dhui thanks a lot for the review. I have addressed the comments. pls check |
dhui
reviewed
Aug 21, 2025
migration.go
Outdated
| @@ -127,34 +128,41 @@ func (m *Migration) Buffer() error { | |||
|
|
|||
| b := bufio.NewReaderSize(m.Body, int(m.BufferSize)) | |||
|
|
|||
| // defer closing buffer writer and body. | |||
| // defer blocks run in reverse order | |||
There was a problem hiding this comment.
Can we use a single defer instead to avoid this confusion?
migration.go
Outdated
| @@ -118,7 +119,7 @@ func (m *Migration) LogString() string { | |||
|
|
|||
| // Buffer buffers Body up to BufferSize. | |||
| // Calling this function blocks. Call with goroutine. | |||
| func (m *Migration) Buffer() error { | |||
| func (m *Migration) Buffer() (err error) { | |||
There was a problem hiding this comment.
Rename this so we can continue to use err below
There was a problem hiding this comment.
done. renamed to berr
|
@dhui addressed the comments. plese check once |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates the Migration.Buffer() function to always close the bufferWriter using a deferred call, even if an error occurs during migration buffering.
We have big migration files and once in a while we saw database driver getting stuck on reading the buffered body.
Below is the code snippet in migrate.go where after printing "Read and execute" statement, the migration got stuck and did not proceed further
This PR introduces change to always close bufferWriter so that BufferedBody readers are not blocked in case Buffer() method returns without closing the bufferWriter.