[Merged by Bors] - Fix nil pointer panic on error

[fasmat]

Motivation

Closes #5034

Changes

• do not call tmp.Name() on error since tmp might be nil.

Test Plan

n/a

TODO

• Explain motivation or link existing issue(s)
• Test changes and document test plan
• Update documentation as needed
• Update changelog as needed

[codecov]

Codecov Report

Merging #5035 (ee39a22) into develop (27c3dd7) will increase coverage by 0.0%.
Report is 3 commits behind head on develop.
The diff coverage is 36.3%.

@@           Coverage Diff           @@
##           develop   #5035   +/-   ##
=======================================
  Coverage     77.0%   77.0%           
=======================================
  Files          257     257           
  Lines        30242   30242           
=======================================
+ Hits         23314   23315    +1     
  Misses        5401    5401           
+ Partials      1527    1526    -1     

Files Changed	Coverage Δ
activation/nipost_state.go	64.2% <36.3%> (+1.5%)	⬆️

... and 3 files with indirect coverage changes

[poszu]

How about using atomic.WriteFile() that takes an io.Reader instead? It should simplify the code a lot (no need to remember to close the temporary file for instance).

[fasmat]

It would make the code a bit shorter, but at the expense of requiring more memory:

	checksum := crc64.New(crc64.MakeTable(crc64.ISO))
	if _, err := checksum.Write(data); err != nil {
		return fmt.Errorf("calc checksum: %w", err)
	}

	buf := &bytes.Buffer{}
	if _, err := buf.Write(data); err != nil {
		return fmt.Errorf("write data %w", err)
	}

	crc := make([]byte, crc64.Size)
	binary.BigEndian.PutUint64(crc, checksum.Sum64())
	if _, err := buf.Write(crc); err != nil {
		return fmt.Errorf("write checksum %w", err)
	}

	if err := atomic.WriteFile(path, buf); err != nil {
		return fmt.Errorf("write file %s: %w", path, err)
	}

	return nil

vs

	tmpName := fmt.Sprintf("%s.tmp", path)
	tmp, err := os.Create(tmpName)
	if err != nil {
		return fmt.Errorf("create temporary file %s: %w", tmpName, err)
	}
	defer tmp.Close()

	checksum := crc64.New(crc64.MakeTable(crc64.ISO))
	w := io.MultiWriter(tmp, checksum)
	if _, err := w.Write(data); err != nil {
		return fmt.Errorf("write data %v: %w", tmp.Name(), err)
	}

	crc := make([]byte, crc64.Size)
	binary.BigEndian.PutUint64(crc, checksum.Sum64())
	if _, err := tmp.Write(crc); err != nil {
		return fmt.Errorf("write checksum %s: %w", tmp.Name(), err)
	}

	if err := tmp.Close(); err != nil {
		return fmt.Errorf("failed to close tmp file %s: %w", tmp.Name(), err)
	}

	if err := atomic.ReplaceFile(tmp.Name(), path); err != nil {
		return fmt.Errorf("save file from %s, %s: %w", tmp.Name(), path, err)
	}

	return nil

[fasmat]

bors merge

[poszu]

@fasmat I was thinking about something like this:

func write(path string, data []byte) error {
	buf := bytes.NewBuffer(nil)
	checksum := crc64.New(crc64.MakeTable(crc64.ISO))
	w := io.MultiWriter(buf, checksum)
	if _, err := w.Write(data); err != nil {
		return fmt.Errorf("write data %v: %w", path, err)
	}

	if _, err := buf.Write(checksum.Sum(buf.AvailableBuffer())); err != nil {
		return fmt.Errorf("write checksum %v: %w", path, err)
	}
	if err := atomic.WriteFile(path, buf); err != nil {
		return fmt.Errorf("save file %v: %w", path, err)
	}
	return nil
}

which I think is close to the original implementation by Kimmy.

💡 There is no need for checksum.Sum64() + big-endian serialization as checksum.Sum() already returns an 8B big-endian checksum.

[bors]

Pull request successfully merged into develop.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• ci-status
• systest-status

[fasmat]

@poszu this is the benchmark I ran:

func BenchmarkWriteCRC(b *testing.B) {
	path := filepath.Join(b.TempDir(), "test.bin")
	data := make([]byte, 4096)
	rand.Read(data)

	for i := 0; i < b.N; i++ {
		require.NoError(b, write(path, data))
	}
}

with your suggested implementation I get

goos: linux
goarch: arm64
pkg: github.com/spacemeshos/go-spacemesh/activation
BenchmarkWriteCRC-6   	   56580	     21884 ns/op	   13584 B/op	      21 allocs/op
PASS
ok  	github.com/spacemeshos/go-spacemesh/activation	1.484s

with the current implementation I get

goos: linux
goarch: arm64
pkg: github.com/spacemeshos/go-spacemesh/activation
BenchmarkWriteCRC-6   	   70664	     16834 ns/op	     680 B/op	      12 allocs/op
PASS
ok  	github.com/spacemeshos/go-spacemesh/activation	1.389s

The reason is that the current implementation doesn't require a buffer that the data is written to before it is written to disk, but rather writes it to disk directly.

EDIT:

💡 There is no need for checksum.Sum64() + big-endian serialization as checksum.Sum() already returns an 8B big-endian checksum.

using checksum.Sum(nil) or even checksum.Sum(crc) (with crc := make([]byte, 0, crc64.Size)) gives me about the same result as the current implementation