op-service: Make target destination when writing JSON/binary explicit (#11800)

Avoids being surprised by the special handling for - and empty string output paths.

cannon/cmd/load_elf.go

@@ -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"
@@ -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)

cannon/cmd/run.go

@@ -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"
@@ -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 {
@@ -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)
 		}
 	}

cannon/serialize/binary.go

@@ -4,7 +4,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"os"
 	"reflect"
 
 	"github.com/ethereum-optimism/optimism/op-service/ioutil"
@@ -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
-	}
-	var out io.Writer
-	finish := func() error { return nil }
-	if outputPath == "-" {
-		out = os.Stdout
-	} else {
-		f, err := ioutil.NewAtomicWriterCompressed(outputPath, perm)
+func WriteSerializedBinary(value Serializable, target ioutil.OutputTarget) error {
+	out, closer, abort, err := target()
 	if err != nil {
-			return fmt.Errorf("failed to create temp file when writing: %w", err)
+		return 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

cannon/serialize/binary_test.go

@@ -7,6 +7,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/ethereum-optimism/optimism/op-service/ioutil"
 	"github.com/stretchr/testify/require"
 )
 
@@ -14,7 +15,7 @@ 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)
@@ -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)

cannon/serialize/detect.go

@@ -4,6 +4,7 @@ import (
 	"os"
 	"strings"
 
+	"github.com/ethereum-optimism/optimism/op-service/ioutil"
 	"github.com/ethereum-optimism/optimism/op-service/jsonutil"
 )
 
@@ -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 {

op-node/cmd/genesis/cmd.go

@@ -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"
@@ -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))
 		},
 	},
 	{
@@ -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))
 		},
 	},
 }

op-service/ioutil/streams.go

+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
+}

op-service/ioutil/streams_test.go

+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)
+	})
+}

op-service/jsonutil/json.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"os"
 
 	"github.com/ethereum-optimism/optimism/op-service/ioutil"
 )
@@ -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
-	}
-	var out io.Writer
-	finish := func() error { return nil }
-	if outputPath == "-" {
-		out = os.Stdout
-	} else {
-		f, err := ioutil.NewAtomicWriterCompressed(outputPath, perm)
+func WriteJSON[X any](value X, target ioutil.OutputTarget) error {
+	out, closer, abort, err := target()
 	if err != nil {
-			return fmt.Errorf("failed to open output file: %w", err)
+		return 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

op-service/jsonutil/json_test.go

@@ -7,6 +7,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/ethereum-optimism/optimism/op-service/ioutil"
 	"github.com/stretchr/testify/require"
 )
 
@@ -14,7 +15,7 @@ func TestRoundTripJSON(t *testing.T) {
 	dir := t.TempDir()
 	file := filepath.Join(dir, "test.json")
 	data := &jsonTestData{A: "yay", B: 3}
-	err := WriteJSON(file, data, 0o755)
+	err := WriteJSON(data, ioutil.ToAtomicFile(file, 0o755))
 	require.NoError(t, err)
 
 	// Confirm the file is uncompressed
@@ -33,7 +34,7 @@ func TestRoundTripJSONWithGzip(t *testing.T) {
 	dir := t.TempDir()
 	file := filepath.Join(dir, "test.json.gz")
 	data := &jsonTestData{A: "yay", B: 3}
-	err := WriteJSON(file, data, 0o755)
+	err := WriteJSON(data, ioutil.ToAtomicFile(file, 0o755))
 	require.NoError(t, err)
 
 	// Confirm the file isn't raw JSON
@@ -87,7 +88,7 @@ func TestLoadJSONWithExtraDataAppended(t *testing.T) {
 			require.NoError(t, err)
 
 			// Write primary json payload + extra data to the file
-			err = WriteJSON(file, data, 0o755)
+			err = WriteJSON(data, ioutil.ToAtomicFile(file, 0o755))
 			require.NoError(t, err)
 			err = appendDataToFile(file, extraData)
 			require.NoError(t, err)
@@ -130,7 +131,7 @@ func TestLoadJSONWithTrailingWhitespace(t *testing.T) {
 			data := &jsonTestData{A: "yay", B: 3}
 
 			// Write primary json payload + extra data to the file
-			err := WriteJSON(file, data, 0o755)
+			err := WriteJSON(data, ioutil.ToAtomicFile(file, 0o755))
 			require.NoError(t, err)
 			err = appendDataToFile(file, tc.extraData)
 			require.NoError(t, err)

op-supervisor/supervisor/backend/db/heads/heads.go

@@ -7,6 +7,7 @@ import (
 	"os"
 	"sync"
 
+	"github.com/ethereum-optimism/optimism/op-service/ioutil"
 	"github.com/ethereum-optimism/optimism/op-service/jsonutil"
 )
 
@@ -58,7 +59,7 @@ func (t *HeadTracker) Current() *Heads {
 }
 
 func (t *HeadTracker) write(heads *Heads) error {
-	if err := jsonutil.WriteJSON(t.path, heads, 0o644); err != nil {
+	if err := jsonutil.WriteJSON(heads, ioutil.ToAtomicFile(t.path, 0o644)); err != nil {
 		return fmt.Errorf("failed to write new heads: %w", err)
 	}
 	return nil