From b6671e2d02fef7063a9364cc8af17ef3152f6c20 Mon Sep 17 00:00:00 2001 From: Aditya Vyas Date: Thu, 25 Jan 2024 12:11:01 -0500 Subject: [PATCH] soroban-rpc: Remove panics from internal codebase (#1167) * Remove panic - 1 * Remove panic - 2 * Remove panic - 3 * Remove panic - 4 * Small changes - 1 * undo changes in Get() func * undo changes - 2 * undo changes - 3 * add test for append error --- .../internal/config/config_option.go | 4 +- .../internal/config/config_option_test.go | 11 ++ cmd/soroban-rpc/internal/config/log_format.go | 12 +- cmd/soroban-rpc/internal/config/options.go | 2 +- .../ledgerbucketwindow/ledgerbucketwindow.go | 6 +- .../ledgerbucketwindow_test.go | 123 +++++++----------- .../internal/transactions/transactions.go | 5 +- 7 files changed, 73 insertions(+), 90 deletions(-) diff --git a/cmd/soroban-rpc/internal/config/config_option.go b/cmd/soroban-rpc/internal/config/config_option.go index 86eab8e7b4..3c8ca87a4e 100644 --- a/cmd/soroban-rpc/internal/config/config_option.go +++ b/cmd/soroban-rpc/internal/config/config_option.go @@ -88,8 +88,6 @@ func (o *ConfigOption) setValue(i interface{}) (err error) { if o.CustomSetValue != nil { return o.CustomSetValue(o, i) } - // it's unfortunate that Set below panics when it cannot set the value.. - // we'll want to catch this so that we can alert the user nicely. defer func() { if recoverRes := recover(); recoverRes != nil { var ok bool @@ -101,7 +99,7 @@ func (o *ConfigOption) setValue(i interface{}) (err error) { } }() parser := func(option *ConfigOption, i interface{}) error { - panic(fmt.Sprintf("no parser for flag %s", o.Name)) + return errors.Errorf("no parser for flag %s", o.Name) } switch o.ConfigKey.(type) { case *bool: diff --git a/cmd/soroban-rpc/internal/config/config_option_test.go b/cmd/soroban-rpc/internal/config/config_option_test.go index 831c886560..a6309cb3e9 100644 --- a/cmd/soroban-rpc/internal/config/config_option_test.go +++ b/cmd/soroban-rpc/internal/config/config_option_test.go @@ -4,6 +4,7 @@ import ( "fmt" "math" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -115,6 +116,16 @@ func TestUnassignableField(t *testing.T) { require.Contains(t, err.Error(), co.Name) } +func TestNoParserForFlag(t *testing.T) { + var co ConfigOption + var invalidKey []time.Duration + co.Name = "mykey" + co.ConfigKey = &invalidKey + err := co.setValue("abc") + require.Error(t, err) + require.Contains(t, err.Error(), "no parser for flag mykey") +} + func TestSetValue(t *testing.T) { var b bool var i int diff --git a/cmd/soroban-rpc/internal/config/log_format.go b/cmd/soroban-rpc/internal/config/log_format.go index 076e43e68b..1ab4c7fc64 100644 --- a/cmd/soroban-rpc/internal/config/log_format.go +++ b/cmd/soroban-rpc/internal/config/log_format.go @@ -1,6 +1,8 @@ package config -import "fmt" +import ( + "fmt" +) type LogFormat int @@ -47,13 +49,13 @@ func (f *LogFormat) UnmarshalTOML(i interface{}) error { } } -func (f LogFormat) String() string { +func (f LogFormat) String() (string, error) { switch f { case LogFormatText: - return "text" + return "text", nil case LogFormatJSON: - return "json" + return "json", nil default: - panic(fmt.Sprintf("unknown log format: %d", f)) + return "", fmt.Errorf("unknown log format: %d", f) } } diff --git a/cmd/soroban-rpc/internal/config/options.go b/cmd/soroban-rpc/internal/config/options.go index cecfb2e7d9..d38dd7cd5b 100644 --- a/cmd/soroban-rpc/internal/config/options.go +++ b/cmd/soroban-rpc/internal/config/options.go @@ -130,7 +130,7 @@ func (cfg *Config) options() ConfigOptions { return nil }, MarshalTOML: func(option *ConfigOption) (interface{}, error) { - return cfg.LogFormat.String(), nil + return cfg.LogFormat.String() }, }, { diff --git a/cmd/soroban-rpc/internal/ledgerbucketwindow/ledgerbucketwindow.go b/cmd/soroban-rpc/internal/ledgerbucketwindow/ledgerbucketwindow.go index 0d447e7163..8234b60765 100644 --- a/cmd/soroban-rpc/internal/ledgerbucketwindow/ledgerbucketwindow.go +++ b/cmd/soroban-rpc/internal/ledgerbucketwindow/ledgerbucketwindow.go @@ -35,12 +35,12 @@ func NewLedgerBucketWindow[T any](retentionWindow uint32) *LedgerBucketWindow[T] } // Append adds a new bucket to the window. If the window is full a bucket will be evicted and returned. -func (w *LedgerBucketWindow[T]) Append(bucket LedgerBucket[T]) *LedgerBucket[T] { +func (w *LedgerBucketWindow[T]) Append(bucket LedgerBucket[T]) (*LedgerBucket[T], error) { length := w.Len() if length > 0 { expectedLedgerSequence := w.buckets[w.start].LedgerSeq + length if expectedLedgerSequence != bucket.LedgerSeq { - panic(fmt.Errorf("ledgers not contiguous: expected ledger sequence %v but received %v", expectedLedgerSequence, bucket.LedgerSeq)) + return &LedgerBucket[T]{}, fmt.Errorf("error appending ledgers: ledgers not contiguous: expected ledger sequence %v but received %v", expectedLedgerSequence, bucket.LedgerSeq) } } @@ -57,7 +57,7 @@ func (w *LedgerBucketWindow[T]) Append(bucket LedgerBucket[T]) *LedgerBucket[T] w.start = (w.start + 1) % length } - return evicted + return evicted, nil } // Len returns the length (number of buckets in the window) diff --git a/cmd/soroban-rpc/internal/ledgerbucketwindow/ledgerbucketwindow_test.go b/cmd/soroban-rpc/internal/ledgerbucketwindow/ledgerbucketwindow_test.go index b472af0b45..2e50ed6d53 100644 --- a/cmd/soroban-rpc/internal/ledgerbucketwindow/ledgerbucketwindow_test.go +++ b/cmd/soroban-rpc/internal/ledgerbucketwindow/ledgerbucketwindow_test.go @@ -18,118 +18,87 @@ func TestAppend(t *testing.T) { m := NewLedgerBucketWindow[uint32](3) require.Equal(t, uint32(0), m.Len()) - // test appending first bucket of events - evicted := m.Append(bucket(5)) + // Test appending first bucket of events + evicted, err := m.Append(bucket(5)) + require.NoError(t, err) require.Nil(t, evicted) require.Equal(t, uint32(1), m.Len()) require.Equal(t, bucket(5), *m.Get(0)) - // the next bucket must follow the previous bucket (ledger 5) - - require.PanicsWithError( - t, "ledgers not contiguous: expected ledger sequence 6 but received 10", - func() { - m.Append(LedgerBucket[uint32]{ - LedgerSeq: 10, - LedgerCloseTimestamp: 100, - BucketContent: 10, - }) - }, - ) - require.PanicsWithError( - t, "ledgers not contiguous: expected ledger sequence 6 but received 4", - func() { - m.Append(LedgerBucket[uint32]{ - LedgerSeq: 4, - LedgerCloseTimestamp: 100, - BucketContent: 4, - }) - }, - ) - require.PanicsWithError( - t, "ledgers not contiguous: expected ledger sequence 6 but received 5", - func() { - m.Append(LedgerBucket[uint32]{ - LedgerSeq: 5, - LedgerCloseTimestamp: 100, - BucketContent: 5, - }) - }, - ) + // The next bucket must follow the previous bucket (ledger 5) + _, err = m.Append(LedgerBucket[uint32]{ + LedgerSeq: 10, + LedgerCloseTimestamp: 100, + BucketContent: 10, + }) + require.Errorf(t, err, "ledgers not contiguous: expected ledger sequence 6 but received 10") + + _, err = m.Append(LedgerBucket[uint32]{ + LedgerSeq: 4, + LedgerCloseTimestamp: 100, + BucketContent: 4, + }) + require.Errorf(t, err, "ledgers not contiguous: expected ledger sequence 6 but received 4") + // check that none of the calls above modified our buckets require.Equal(t, uint32(1), m.Len()) require.Equal(t, bucket(5), *m.Get(0)) - // append ledger 6 bucket, now we have two buckets filled - evicted = m.Append(bucket(6)) + // Append ledger 6 bucket, now we have two buckets filled + evicted, err = m.Append(bucket(6)) + require.NoError(t, err) require.Nil(t, evicted) require.Equal(t, uint32(2), m.Len()) require.Equal(t, bucket(5), *m.Get(0)) require.Equal(t, bucket(6), *m.Get(1)) - // the next bucket of events must follow the previous bucket (ledger 6) - require.PanicsWithError( - t, "ledgers not contiguous: expected ledger sequence 7 but received 10", - func() { - m.Append(LedgerBucket[uint32]{ - LedgerSeq: 10, - LedgerCloseTimestamp: 100, - BucketContent: 10, - }) - }, - ) - require.PanicsWithError( - t, "ledgers not contiguous: expected ledger sequence 7 but received 4", - func() { - m.Append(LedgerBucket[uint32]{ - LedgerSeq: 4, - LedgerCloseTimestamp: 100, - BucketContent: 4, - }) - }, - ) - require.PanicsWithError( - t, "ledgers not contiguous: expected ledger sequence 7 but received 5", - func() { - m.Append(LedgerBucket[uint32]{ - LedgerSeq: 5, - LedgerCloseTimestamp: 100, - BucketContent: 5, - }) - }, - ) - - // append ledger 7, now we have all three buckets filled - evicted = m.Append(bucket(7)) - require.Nil(t, evicted) + // Append ledger 7, now we have all three buckets filled + evicted, err = m.Append(bucket(7)) + require.NoError(t, err) require.Nil(t, evicted) require.Equal(t, uint32(3), m.Len()) require.Equal(t, bucket(5), *m.Get(0)) require.Equal(t, bucket(6), *m.Get(1)) require.Equal(t, bucket(7), *m.Get(2)) - // append ledger 8, but all buckets are full, so we need to evict ledger 5 - evicted = m.Append(bucket(8)) + // Append ledger 8, but all buckets are full, so we need to evict ledger 5 + evicted, err = m.Append(bucket(8)) + require.NoError(t, err) require.Equal(t, bucket(5), *evicted) require.Equal(t, uint32(3), m.Len()) require.Equal(t, bucket(6), *m.Get(0)) require.Equal(t, bucket(7), *m.Get(1)) require.Equal(t, bucket(8), *m.Get(2)) - // append ledger 9 events, but all buckets are full, so we need to evict ledger 6 - evicted = m.Append(bucket(9)) + // Append ledger 9 events, but all buckets are full, so we need to evict ledger 6 + evicted, err = m.Append(bucket(9)) + require.NoError(t, err) require.Equal(t, bucket(6), *evicted) require.Equal(t, uint32(3), m.Len()) require.Equal(t, bucket(7), *m.Get(0)) require.Equal(t, bucket(8), *m.Get(1)) require.Equal(t, bucket(9), *m.Get(2)) - // append ledger 10, but all buckets are full, so we need to evict ledger 7. + // Append ledger 10, but all buckets are full, so we need to evict ledger 7. // The start index must have wrapped around - evicted = m.Append(bucket(10)) + evicted, err = m.Append(bucket(10)) + require.NoError(t, err) require.Equal(t, bucket(7), *evicted) require.Equal(t, uint32(3), m.Len()) require.Equal(t, bucket(8), *m.Get(0)) require.Equal(t, bucket(9), *m.Get(1)) require.Equal(t, bucket(10), *m.Get(2)) } + +func TestAppendError(t *testing.T) { + m := NewLedgerBucketWindow[uint32](3) + require.Equal(t, uint32(0), m.Len()) + + evicted, err := m.Append(bucket(5)) + require.NoError(t, err) + require.Nil(t, evicted) + + evicted, err = m.Append(bucket(1)) + require.Error(t, err) + require.Contains(t, err.Error(), "error appending ledgers: ledgers not contiguous: expected ledger sequence 6 but received 1") +} diff --git a/cmd/soroban-rpc/internal/transactions/transactions.go b/cmd/soroban-rpc/internal/transactions/transactions.go index 8d58a035a1..5ed719ade6 100644 --- a/cmd/soroban-rpc/internal/transactions/transactions.go +++ b/cmd/soroban-rpc/internal/transactions/transactions.go @@ -122,7 +122,10 @@ func (m *MemoryStore) IngestTransactions(ledgerCloseMeta xdr.LedgerCloseMeta) er m.lock.Lock() defer m.lock.Unlock() - evicted := m.transactionsByLedger.Append(bucket) + evicted, err := m.transactionsByLedger.Append(bucket) + if err != nil { + return err + } if evicted != nil { // garbage-collect evicted entries for _, evictedTxHash := range evicted.BucketContent {