Skip to content

Commit

Permalink
op-service: Make target destination when writing JSON/binary explicit (
Browse files Browse the repository at this point in the history
…#11800)

Avoids being surprised by the special handling for - and empty string output paths.
  • Loading branch information
ajsutton authored Sep 9, 2024
1 parent a0d3195 commit f64d817
Show file tree
Hide file tree
Showing 11 changed files with 192 additions and 61 deletions.
3 changes: 2 additions & 1 deletion cannon/cmd/load_elf.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/ethereum-optimism/optimism/cannon/mipsevm"
"github.com/ethereum-optimism/optimism/cannon/mipsevm/multithreaded"
"github.com/ethereum-optimism/optimism/cannon/serialize"
"github.com/ethereum-optimism/optimism/op-service/ioutil"
"github.com/urfave/cli/v2"

"github.com/ethereum-optimism/optimism/cannon/mipsevm/program"
Expand Down Expand Up @@ -93,7 +94,7 @@ func LoadELF(ctx *cli.Context) error {
if err != nil {
return fmt.Errorf("failed to compute program metadata: %w", err)
}
if err := jsonutil.WriteJSON[*program.Metadata](ctx.Path(LoadELFMetaFlag.Name), meta, OutFilePerm); err != nil {
if err := jsonutil.WriteJSON[*program.Metadata](meta, ioutil.ToStdOutOrFileOrNoop(ctx.Path(LoadELFMetaFlag.Name), OutFilePerm)); err != nil {
return fmt.Errorf("failed to output metadata: %w", err)
}
return writeState(ctx.Path(LoadELFOutFlag.Name), state)
Expand Down
5 changes: 3 additions & 2 deletions cannon/cmd/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/ethereum-optimism/optimism/cannon/mipsevm/multithreaded"
"github.com/ethereum-optimism/optimism/cannon/serialize"
"github.com/ethereum-optimism/optimism/op-service/ioutil"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/common/hexutil"
"github.com/ethereum/go-ethereum/log"
Expand Down Expand Up @@ -478,7 +479,7 @@ func Run(ctx *cli.Context) error {
proof.OracleValue = witness.PreimageValue
proof.OracleOffset = witness.PreimageOffset
}
if err := jsonutil.WriteJSON(fmt.Sprintf(proofFmt, step), proof, OutFilePerm); err != nil {
if err := jsonutil.WriteJSON(proof, ioutil.ToStdOutOrFileOrNoop(fmt.Sprintf(proofFmt, step), OutFilePerm)); err != nil {
return fmt.Errorf("failed to write proof data: %w", err)
}
} else {
Expand Down Expand Up @@ -516,7 +517,7 @@ func Run(ctx *cli.Context) error {
return fmt.Errorf("failed to write state output: %w", err)
}
if debugInfoFile := ctx.Path(RunDebugInfoFlag.Name); debugInfoFile != "" {
if err := jsonutil.WriteJSON(debugInfoFile, vm.GetDebugInfo(), OutFilePerm); err != nil {
if err := jsonutil.WriteJSON(vm.GetDebugInfo(), ioutil.ToStdOutOrFileOrNoop(debugInfoFile, OutFilePerm)); err != nil {
return fmt.Errorf("failed to write benchmark data: %w", err)
}
}
Expand Down
32 changes: 9 additions & 23 deletions cannon/serialize/binary.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"errors"
"fmt"
"io"
"os"
"reflect"

"github.com/ethereum-optimism/optimism/op-service/ioutil"
Expand Down Expand Up @@ -42,33 +41,20 @@ func LoadSerializedBinary[X any](inputPath string) (*X, error) {
return &x, nil
}

func WriteSerializedBinary(outputPath string, value Serializable, perm os.FileMode) error {
if outputPath == "" {
return nil
func WriteSerializedBinary(value Serializable, target ioutil.OutputTarget) error {
out, closer, abort, err := target()
if err != nil {
return err
}
var out io.Writer
finish := func() error { return nil }
if outputPath == "-" {
out = os.Stdout
} else {
f, err := ioutil.NewAtomicWriterCompressed(outputPath, perm)
if err != nil {
return fmt.Errorf("failed to create temp file when writing: %w", err)
}
// Ensure we close the stream without renaming even if failures occur.
defer func() {
_ = f.Abort()
}()
out = f
// Closing the file causes it to be renamed to the final destination
// so make sure we handle any errors it returns
finish = f.Close
if out == nil {
return nil // Nothing to write to so skip generating content entirely
}
err := value.Serialize(out)
defer abort()
err = value.Serialize(out)
if err != nil {
return fmt.Errorf("failed to write binary: %w", err)
}
if err := finish(); err != nil {
if err := closer.Close(); err != nil {
return fmt.Errorf("failed to finish write: %w", err)
}
return nil
Expand Down
5 changes: 3 additions & 2 deletions cannon/serialize/binary_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@ import (
"path/filepath"
"testing"

"github.com/ethereum-optimism/optimism/op-service/ioutil"
"github.com/stretchr/testify/require"
)

func TestRoundTripBinary(t *testing.T) {
dir := t.TempDir()
file := filepath.Join(dir, "test.bin")
data := &serializableTestData{A: []byte{0xde, 0xad}, B: 3}
err := WriteSerializedBinary(file, data, 0644)
err := WriteSerializedBinary(data, ioutil.ToAtomicFile(file, 0644))
require.NoError(t, err)

hasGzip, err := hasGzipHeader(file)
Expand All @@ -30,7 +31,7 @@ func TestRoundTripBinaryWithGzip(t *testing.T) {
dir := t.TempDir()
file := filepath.Join(dir, "test.bin.gz")
data := &serializableTestData{A: []byte{0xde, 0xad}, B: 3}
err := WriteSerializedBinary(file, data, 0644)
err := WriteSerializedBinary(data, ioutil.ToAtomicFile(file, 0644))
require.NoError(t, err)

hasGzip, err := hasGzipHeader(file)
Expand Down
5 changes: 3 additions & 2 deletions cannon/serialize/detect.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"os"
"strings"

"github.com/ethereum-optimism/optimism/op-service/ioutil"
"github.com/ethereum-optimism/optimism/op-service/jsonutil"
)

Expand All @@ -16,9 +17,9 @@ func Load[X any](inputPath string) (*X, error) {

func Write[X Serializable](outputPath string, x X, perm os.FileMode) error {
if isBinary(outputPath) {
return WriteSerializedBinary(outputPath, x, perm)
return WriteSerializedBinary(x, ioutil.ToStdOutOrFileOrNoop(outputPath, perm))
}
return jsonutil.WriteJSON[X](outputPath, x, perm)
return jsonutil.WriteJSON[X](x, ioutil.ToStdOutOrFileOrNoop(outputPath, perm))
}

func isBinary(path string) bool {
Expand Down
7 changes: 4 additions & 3 deletions op-node/cmd/genesis/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"time"

"github.com/ethereum-optimism/optimism/op-service/ioutil"
"github.com/ethereum-optimism/optimism/op-service/retry"
"github.com/ethereum-optimism/optimism/op-service/sources/batching"
"github.com/urfave/cli/v2"
Expand Down Expand Up @@ -120,7 +121,7 @@ var Subcommands = cli.Commands{
return err
}

return jsonutil.WriteJSON(ctx.String(outfileL1Flag.Name), l1Genesis, 0o666)
return jsonutil.WriteJSON(l1Genesis, ioutil.ToStdOutOrFileOrNoop(ctx.String(outfileL1Flag.Name), 0o666))
},
},
{
Expand Down Expand Up @@ -204,10 +205,10 @@ var Subcommands = cli.Commands{
return fmt.Errorf("generated rollup config does not pass validation: %w", err)
}

if err := jsonutil.WriteJSON(ctx.String(outfileL2Flag.Name), l2Genesis, 0o666); err != nil {
if err := jsonutil.WriteJSON(l2Genesis, ioutil.ToAtomicFile(ctx.String(outfileL2Flag.Name), 0o666)); err != nil {
return err
}
return jsonutil.WriteJSON(ctx.String(outfileRollupFlag.Name), rollupConfig, 0o666)
return jsonutil.WriteJSON(rollupConfig, ioutil.ToAtomicFile(ctx.String(outfileRollupFlag.Name), 0o666))
},
},
}
52 changes: 52 additions & 0 deletions op-service/ioutil/streams.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package ioutil

import (
"io"
"os"
)

var (
stdOutStream OutputTarget = func() (io.Writer, io.Closer, Aborter, error) {
return os.Stdout, &noopCloser{}, func() {}, nil
}
)

type Aborter func()

type OutputTarget func() (io.Writer, io.Closer, Aborter, error)

func NoOutputStream() OutputTarget {
return func() (io.Writer, io.Closer, Aborter, error) {
return nil, nil, nil, nil
}
}

func ToAtomicFile(path string, perm os.FileMode) OutputTarget {
return func() (io.Writer, io.Closer, Aborter, error) {
f, err := NewAtomicWriterCompressed(path, perm)
if err != nil {
return nil, nil, nil, err
}
return f, f, func() { _ = f.Abort() }, nil
}
}

func ToStdOut() OutputTarget {
return stdOutStream
}

func ToStdOutOrFileOrNoop(outputPath string, perm os.FileMode) OutputTarget {
if outputPath == "" {
return NoOutputStream()
} else if outputPath == "-" {
return ToStdOut()
} else {
return ToAtomicFile(outputPath, perm)
}
}

type noopCloser struct{}

func (c *noopCloser) Close() error {
return nil
}
100 changes: 100 additions & 0 deletions op-service/ioutil/streams_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
package ioutil

import (
"os"
"path/filepath"
"testing"

"github.com/stretchr/testify/require"
)

func TestNoOutputStream(t *testing.T) {
writer, closer, aborter, err := NoOutputStream()()
require.NoError(t, err)
require.Nil(t, writer)
require.Nil(t, closer)
require.Nil(t, aborter)
}

func TestToStdOut(t *testing.T) {
writer, closer, aborter, err := ToStdOut()()
require.NoError(t, err)
require.Same(t, os.Stdout, writer)

// Should not close StdOut
require.NoError(t, closer.Close())
_, err = os.Stdout.WriteString("TestToStdOut After Close\n")
require.NoError(t, err)

aborter()
_, err = os.Stdout.WriteString("TestToStdOut After Abort\n")
require.NoError(t, err)
}

func TestToAtomicFile(t *testing.T) {
t.Run("Abort", func(t *testing.T) {
dir := t.TempDir()
path := filepath.Join(dir, "test.txt")
writer, closer, aborter, err := ToAtomicFile(path, 0o644)()
defer closer.Close()
require.NoError(t, err)

expected := []byte("test")
_, err = writer.Write(expected)
require.NoError(t, err)
aborter()

_, err = os.Stat(path)
require.ErrorIs(t, err, os.ErrNotExist, "Should not have written file")
})

t.Run("Close", func(t *testing.T) {
dir := t.TempDir()
path := filepath.Join(dir, "test.txt")
writer, closer, _, err := ToAtomicFile(path, 0o644)()
defer closer.Close()
require.NoError(t, err)

expected := []byte("test")
_, err = writer.Write(expected)
require.NoError(t, err)

_, err = os.Stat(path)
require.ErrorIs(t, err, os.ErrNotExist, "Target file should not exist prior to Close")

require.NoError(t, closer.Close())
actual, err := os.ReadFile(path)
require.NoError(t, err)
require.Equal(t, expected, actual)
})
}

func TestToStdOutOrFileOrNoop(t *testing.T) {
t.Run("EmptyOutputPath", func(t *testing.T) {
writer, _, _, err := ToStdOutOrFileOrNoop("", 0o644)()
require.NoError(t, err)
require.Nil(t, writer, "Should use no output stream")
})

t.Run("StdOut", func(t *testing.T) {
writer, _, _, err := ToStdOutOrFileOrNoop("-", 0o644)()
require.NoError(t, err)
require.Same(t, os.Stdout, writer, "Should use std out")
})

t.Run("File", func(t *testing.T) {
dir := t.TempDir()
path := filepath.Join(dir, "test.txt")
writer, closer, _, err := ToStdOutOrFileOrNoop(path, 0o644)()
defer closer.Close()
require.NoError(t, err)

expected := []byte("test")
_, err = writer.Write(expected)
require.NoError(t, err)
require.NoError(t, closer.Close())
actual, err := os.ReadFile(path)
require.NoError(t, err)
require.Equal(t, expected, actual)
})
}
32 changes: 9 additions & 23 deletions op-service/jsonutil/json.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"errors"
"fmt"
"io"
"os"

"github.com/ethereum-optimism/optimism/op-service/ioutil"
)
Expand All @@ -32,38 +31,25 @@ func LoadJSON[X any](inputPath string) (*X, error) {
return &state, nil
}

func WriteJSON[X any](outputPath string, value X, perm os.FileMode) error {
if outputPath == "" {
return nil
func WriteJSON[X any](value X, target ioutil.OutputTarget) error {
out, closer, abort, err := target()
if err != nil {
return err
}
var out io.Writer
finish := func() error { return nil }
if outputPath == "-" {
out = os.Stdout
} else {
f, err := ioutil.NewAtomicWriterCompressed(outputPath, perm)
if err != nil {
return fmt.Errorf("failed to open output file: %w", err)
}
// Ensure we close the stream without renaming even if failures occur.
defer func() {
_ = f.Abort()
}()
out = f
// Closing the file causes it to be renamed to the final destination
// so make sure we handle any errors it returns
finish = f.Close
if out == nil {
return nil // No output stream selected so skip generating the content entirely
}
defer abort()
enc := json.NewEncoder(out)
enc.SetIndent("", " ")
if err := enc.Encode(value); err != nil {
return fmt.Errorf("failed to encode to JSON: %w", err)
}
_, err := out.Write([]byte{'\n'})
_, err = out.Write([]byte{'\n'})
if err != nil {
return fmt.Errorf("failed to append new-line: %w", err)
}
if err := finish(); err != nil {
if err := closer.Close(); err != nil {
return fmt.Errorf("failed to finish write: %w", err)
}
return nil
Expand Down
Loading

0 comments on commit f64d817

Please sign in to comment.