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.

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

Bugs fixes #18086

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Bugs fixes #18086

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab62ea8
memory leak fix
TheRealMal May 28, 2024
7bad55f
fix nil deref
TheRealMal May 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
memory leak fix
  • Loading branch information
@TheRealMal
TheRealMal authored May 28, 2024
commit ab62ea8f72802d2f58f48884b6835e767279095d
4 changes: 1 addition & 3 deletions client/v3/snapshot/v3_snapshot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ func SaveWithVersion(ctx context.Context, lg *zap.Logger, cfg clientv3.Config, d
if err != nil {
return "", fmt.Errorf("could not open %s (%v)", partpath, err)
}
defer f.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

it sounds like an easy fix, but how would the rename operation below work on an open file?

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL this actually works with the rename syscall on linux. It will fail on Windows with the classic "the process cannot access the file because it is being used by another process". Which is Support Tier 3, but the fix is simple: just wrap the file operations before the rename in a function and the defer within.

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean by "rename operation"? I just moved f.Close() to defer, because file should be closed even if any error occured after it being opened. I can't see any logic changes.

Copy link
Contributor

@tjungblu tjungblu May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This operation:

etcd/client/v3/snapshot/v3_snapshot.go

Lines 95 to 97 in 7bad55f

if err = os.Rename(partpath, dbPath); err != nil {
return resp.Version, fmt.Errorf("could not rename %s to %s (%v)", partpath, dbPath, err)
}

will fail on Windows, because defer closes the file after the function exits and you can't rename a still opened file.

MRE if you want to try this out: https://gist.github.com/tjungblu/7d4ff9bc88afe86004bf292fd9032966

Copy link
Author

Choose a reason for hiding this comment

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

Well, you are right

Copy link
Contributor

Choose a reason for hiding this comment

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

turned out to be slightly more verbose than I thought, but that could work:

	err = func() (err error) {
		var f *os.File
		f, err = os.OpenFile(partpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fileutil.PrivateFileMode)
		if err != nil {
			return fmt.Errorf("could not open %s (%v)", partpath, err)
		}
		defer func() {
			err = f.Close()
		}()
		
		lg.Info("created temporary db file", zap.String("path", partpath))

		var rd io.ReadCloser
		rd, err = cli.Snapshot(ctx)
		if err != nil {
			return
		}
		lg.Info("fetching snapshot", zap.String("endpoint", cfg.Endpoints[0]))
		var size int64
		size, err = io.Copy(f, rd)
		if err != nil {
			return
		}
		if !hasChecksum(size) {
			return fmt.Errorf("sha256 checksum not found [bytes: %d]", size)
		}
		if err = fileutil.Fsync(f); err != nil {
			return
		}

		lg.Info("fetched snapshot",
			zap.String("endpoint", cfg.Endpoints[0]),
			zap.String("size", humanize.Bytes(uint64(size))),
			zap.String("took", humanize.Time(now)),
		)

		return
	}()

	if err != nil {
		return err
	}

Copy link
Author

@TheRealMal TheRealMal May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But err = f.Close() will rewrite any error which comes after defer func() {err = f.Close()}() inside that block, won't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Joining the errors should work :)

Ref: https://pkg.go.dev/errors#Join

lg.Info("created temporary db file", zap.String("path", partpath))

start := time.Now()
Expand All @@ -84,9 +85,6 @@ func SaveWithVersion(ctx context.Context, lg *zap.Logger, cfg clientv3.Config, d
if err = fileutil.Fsync(f); err != nil {
return resp.Version, err
}
if err = f.Close(); err != nil {
return resp.Version, err
}
lg.Info("fetched snapshot",
zap.String("endpoint", cfg.Endpoints[0]),
zap.String("size", humanize.Bytes(uint64(size))),
Expand Down