Skip to content

fix: handle error on Close() of writable objects #2731

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 3, 2025
Merged

fix: handle error on Close() of writable objects #2731

merged 1 commit into from
Mar 3, 2025

Conversation

aeijdenberg
Copy link
Contributor

For objects (particularly files) that we write to, we should to check the return value of f.Close() and pass error back to the caller if this fails.

I recently read https://www.joeshaw.org/dont-defer-close-on-writable-files/ and while working on another PR in this repo I noticed that some some writable files had the defer f.Close() pattern, so I did a grep to find similar calls, and for cases involving writeable file, changed the defer function to capture the result of calling Close(), and if the err value of the parent is nil, then set it for the caller.

In this manner errors on close will be returned to the caller, but won't mask other errors if an error was already set.

This adjusts those to one of the patterns in that article, e.g.:

func doThing() (err error) {
	outFile, err := os.Create(dst)
	if err != nil {
		return err
	}

	// since we are writing to this file, make sure we handle errors
	defer func() {
		closeErr := outFile.Close()
		if err == nil {
			err = closeErr
		}
	}()

        .....
}

mtrmac
mtrmac reviewed Feb 22, 2025
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

Just a quick note for now: This is definitely valuable; but I’ve been bitten by unintended named return value shadowing/overwriting several times in the past. Please use a clearly-unique name for the error value modified by defer (retErr is common in this codebase); for the other return values, it’s best not to add names to them them at all — use _.

Note to self: I didn’t actually review the changes.

@aeijdenberg aeijdenberg force-pushed the fixunhandleclosewritable branch from 07cd6a6 to b41e3f3 Compare February 23, 2025 00:06
@aeijdenberg
Copy link
Contributor Author

Please use a clearly-unique name for the error value modified by defer (retErr is common in this codebase); for the other return values, it’s best not to add names to them them at all — use _.

Thanks, done as suggested, and squashed/rebased. I also removed the change to pkg/blobcache/dest.go - I'll have a go at that in a different PR as the entire method there looked to be quite unique in how it was doing its error handling flow.

mtrmac
mtrmac reviewed Feb 25, 2025
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

In some of the situations, it seems inconsistent to recognize and report an error to write to a file, but to update records as if the file were written successfully anyway: the close + error detection should probably happen a bit earlier.

(Compare other code with “// A scope for defer” comments.)

@aeijdenberg aeijdenberg force-pushed the fixunhandleclosewritable branch from e15cf5d to 187218b Compare February 27, 2025 09:51
mtrmac
mtrmac reviewed Feb 27, 2025
View reviewed changes
mtrmac
mtrmac reviewed Feb 27, 2025
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM otherwise.

@aeijdenberg aeijdenberg force-pushed the fixunhandleclosewritable branch from 187218b to 25d6788 Compare February 28, 2025 06:16
mtrmac
mtrmac approved these changes Feb 28, 2025
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

@mtrmac
Copy link
Collaborator

mtrmac commented Feb 28, 2025

Can you rebase, please?

@aeijdenberg
fix: handle error on Close() of writable objects
bcbb446 
For objects (particularly files) that we write to, we need to check the
return value of f.Close() and pass error back to the caller if this
fails.

Signed-off-by: Adam Eijdenberg <adam@continusec.com>
@aeijdenberg aeijdenberg force-pushed the fixunhandleclosewritable branch from 25d6788 to bcbb446 Compare February 28, 2025 20:22
@mtrmac mtrmac merged commit ee25666 into containers:main Mar 3, 2025
10 checks passed
@mtrmac
Copy link
Collaborator

mtrmac commented Mar 3, 2025

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtrmac mtrmac mtrmac approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aeijdenberg @mtrmac