From 3bb889695cc04f9edcac537e3a4c3e8ee266ba10 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Sat, 21 Jan 2023 12:07:26 +0100 Subject: [PATCH] Address review feedback --- .../internal/ledgerentry_storage/db.go | 22 ++++------ .../internal/ledgerentry_storage/db_test.go | 42 ++++++------------- 2 files changed, 21 insertions(+), 43 deletions(-) diff --git a/cmd/soroban-rpc/internal/ledgerentry_storage/db.go b/cmd/soroban-rpc/internal/ledgerentry_storage/db.go index 47fb3e62f..4bbef4f4c 100644 --- a/cmd/soroban-rpc/internal/ledgerentry_storage/db.go +++ b/cmd/soroban-rpc/internal/ledgerentry_storage/db.go @@ -143,12 +143,11 @@ func (s *sqlDB) GetLatestLedgerSequence() (uint32, error) { if err != nil { return 0, err } + // Since it's a read-only transaction, we don't + // care whether we commit it or roll it back as long as we close it + defer tx.Rollback() ret, err := getLatestLedgerSequence(tx) if err != nil { - _ = tx.Rollback() - return 0, err - } - if err := tx.Commit(); err != nil { return 0, err } return ret, nil @@ -162,24 +161,19 @@ func (s *sqlDB) GetLedgerEntry(key xdr.LedgerKey) (xdr.LedgerEntry, bool, uint32 if err != nil { return xdr.LedgerEntry{}, false, 0, err } + // Since it's a read-only transaction, we don't + // care whether we commit it or roll it back as long as we close it + defer tx.Rollback() seq, err := getLatestLedgerSequence(tx) if err != nil { - _ = tx.Rollback() return xdr.LedgerEntry{}, false, 0, err } buffer := xdr.NewEncodingBuffer() entry, err := getLedgerEntry(tx, buffer, key) if err == sql.ErrNoRows { - if err = tx.Commit(); err != nil { - return xdr.LedgerEntry{}, false, 0, err - } return xdr.LedgerEntry{}, false, seq, nil } if err != nil { - _ = tx.Rollback() - return xdr.LedgerEntry{}, false, seq, err - } - if err := tx.Commit(); err != nil { return xdr.LedgerEntry{}, false, seq, err } return entry, true, seq, nil @@ -255,6 +249,7 @@ func (l *ledgerUpdaterTx) UpsertLedgerEntry(key xdr.LedgerKey, entry xdr.LedgerE // safe since we cast to string right away encodedEntry, err := l.buffer.UnsafeMarshalBinary(&entry) if err != nil { + _ = l.tx.Rollback() return err } encodedEntryStr := string(encodedEntry) @@ -273,6 +268,7 @@ func (l *ledgerUpdaterTx) UpsertLedgerEntry(key xdr.LedgerKey, entry xdr.LedgerE func (l *ledgerUpdaterTx) DeleteLedgerEntry(key xdr.LedgerKey) error { encodedKey, err := encodeLedgerKey(l.buffer, key) if err != nil { + _ = l.tx.Rollback() return err } l.keyToEntryBatch[encodedKey] = nil @@ -299,7 +295,7 @@ func (l *ledgerUpdaterTx) Done() error { if err := l.tx.Commit(); err != nil { return err } - if l.postWriteCommitHook() != nil { + if l.postWriteCommitHook != nil { if err := l.postWriteCommitHook(); err != nil { return err } diff --git a/cmd/soroban-rpc/internal/ledgerentry_storage/db_test.go b/cmd/soroban-rpc/internal/ledgerentry_storage/db_test.go index 84cb53bdf..4feb154fc 100644 --- a/cmd/soroban-rpc/internal/ledgerentry_storage/db_test.go +++ b/cmd/soroban-rpc/internal/ledgerentry_storage/db_test.go @@ -5,7 +5,6 @@ import ( "database/sql" "fmt" "math/rand" - "os" "path" "sync" "testing" @@ -17,10 +16,9 @@ import ( ) func TestGoldenPath(t *testing.T) { - db, dbPath := NewTestDB() + db := NewTestDB(t) defer func() { assert.NoError(t, db.Close()) - assert.NoError(t, os.Remove(dbPath)) }() // Check that we get an empty DB error @@ -101,10 +99,9 @@ func TestGoldenPath(t *testing.T) { } func TestDeleteNonExistentLedgerEmpty(t *testing.T) { - db, dbPath := NewTestDB() + db := NewTestDB(t) defer func() { assert.NoError(t, db.Close()) - assert.NoError(t, os.Remove(dbPath)) }() // Simulate a ledger which creates and deletes a ledger entry @@ -166,10 +163,9 @@ func getContractDataLedgerEntry(data xdr.ContractDataEntry) (xdr.LedgerKey, xdr. // Make sure that (multiple, simultaneous) read transactions can happen while a write-transaction is ongoing, // and write is only visible once the transaction is committed func TestReadTxsDuringWriteTx(t *testing.T) { - db, dbPath := NewTestDB() + db := NewTestDB(t) defer func() { assert.NoError(t, db.Close()) - assert.NoError(t, os.Remove(dbPath)) }() // Check that we get an empty DB error @@ -237,10 +233,9 @@ func TestReadTxsDuringWriteTx(t *testing.T) { // Make sure that a write transaction can happen while multiple read transactions are ongoing, // and write is only visible once the transaction is committed func TestWriteTxsDuringReadTxs(t *testing.T) { - db, dbPath := NewTestDB() + db := NewTestDB(t) defer func() { assert.NoError(t, db.Close()) - assert.NoError(t, os.Remove(dbPath)) }() // Check that we get an empty DB error @@ -320,16 +315,9 @@ func TestWriteTxsDuringReadTxs(t *testing.T) { // Check that we can have coexisting reader and writer goroutines without deadlocks or errors func TestConcurrentReadersAndWriter(t *testing.T) { - db, dbPath := NewTestDB() + db := NewTestDB(t) defer func() { - err := db.Close() - if err != nil { - panic(err) - } - err = os.Remove(dbPath) - if err != nil { - panic(err) - } + assert.NoError(t, db.Close()) }() contractID := xdr.Hash{0xca, 0xfe} done := make(chan struct{}) @@ -410,16 +398,9 @@ func TestConcurrentReadersAndWriter(t *testing.T) { } func BenchmarkLedgerUpdate(b *testing.B) { - db, dbPath := NewTestDB() + db := NewTestDB(b) defer func() { - err := db.Close() - if err != nil { - panic(err) - } - err = os.Remove(dbPath) - if err != nil { - panic(err) - } + assert.NoError(b, db.Close()) }() keyUint32 := xdr.Uint32(0) data := xdr.ContractDataEntry{ @@ -454,11 +435,12 @@ func BenchmarkLedgerUpdate(b *testing.B) { b.StopTimer() } -func NewTestDB() (DB, string) { - dbPath := path.Join(os.TempDir(), fmt.Sprintf("%08x.sqlite", rand.Int63())) +func NewTestDB(tb testing.TB) DB { + tmp := tb.TempDir() + dbPath := path.Join(tmp, "db.sqlite") db, err := OpenSQLiteDB(dbPath) if err != nil { panic(err) } - return db, dbPath + return db }