updated error handling

StephenButtolph · StephenButtolph · commit 9ac020ba0b0b · 2020-08-02T11:26:35.000-04:00
diff --git a/api/admin/performance.go b/api/admin/performance.go
@@ -39,9 +39,7 @@ func (p *Performance) StartCPUProfiler() error {
 		return err
 	}
 	if err := pprof.StartCPUProfile(file); err != nil {
-		// Close should return an error if it has already been called
-		// Return the original error in case of error
-		file.Close() // #nosec G104
+		_ = file.Close() // Return the original error
 		return err
 	}
 	runtime.SetMutexProfileFraction(1)
@@ -70,9 +68,7 @@ func (p *Performance) MemoryProfile() error {
 	}
 	runtime.GC() // get up-to-date statistics
 	if err := pprof.WriteHeapProfile(file); err != nil {
-		// Close should return an error if it has already been called
-		// Return the original error in case of error
-		file.Close() // #nosec G104
+		_ = file.Close() // Return the original error
 		return err
 	}
 	return file.Close()
@@ -87,9 +83,7 @@ func (p *Performance) LockProfile() error {
 
 	profile := pprof.Lookup("mutex")
 	if err := profile.WriteTo(file, 1); err != nil {
-		// Close should return an error if it has already been called
-		// Return the original error in case of error
-		file.Close() // #nosec G104
+		_ = file.Close() // Return the original error
 		return err
 	}
 	return file.Close()
diff --git a/api/ipcs/server.go b/api/ipcs/server.go
@@ -89,9 +89,7 @@ func (ipc *IPCs) PublishBlockchain(r *http.Request, args *PublishBlockchainArgs,
 
 	if err = sock.Listen(url); err != nil {
 		ipc.log.Error("can't listen on pub socket: %s", err)
-		// Closing the socket should only error if it has already been closed
-		// #nosec G104
-		sock.Close()
+		_ = sock.Close() // Return the original error
 		return err
 	}
 
@@ -101,9 +99,7 @@ func (ipc *IPCs) PublishBlockchain(r *http.Request, args *PublishBlockchainArgs,
 	}
 	if err := ipc.events.RegisterChain(chainID, "ipc", chainIPC); err != nil {
 		ipc.log.Error("couldn't register event: %s", err)
-		// Closing the socket should only error if it has already been closed
-		// #nosec G104
-		sock.Close()
+		_ = sock.Close() // Return the original error
 		return err
 	}
 
diff --git a/chains/manager.go b/chains/manager.go
@@ -379,7 +379,7 @@ func (m *manager) buildChain(chainParams ChainParameters) (*chain, error) {
 			ctx.Lock.Lock()
 			defer ctx.Lock.Unlock()
 			if err := chain.Engine.Startup(); err != nil {
-				chain.Ctx.Log.Error("failed to start consensus engine: %w", err)
+				chain.Ctx.Log.Error("failed to start consensus engine: %s", err)
 				chain.Handler.Shutdown()
 			}
 		})
diff --git a/database/memdb/db.go b/database/memdb/db.go
@@ -142,7 +142,15 @@ func (db *Database) NewIteratorWithStartAndPrefix(start, prefix []byte) database
 func (db *Database) Stat(property string) (string, error) { return "", database.ErrNotFound }
 
 // Compact implements the Database interface
-func (db *Database) Compact(start []byte, limit []byte) error { return nil }
+func (db *Database) Compact(start []byte, limit []byte) error {
+	db.lock.RLock()
+	defer db.lock.RUnlock()
+
+	if db.db == nil {
+		return database.ErrClosed
+	}
+	return nil
+}
 
 type keyValue struct {
 	key    []byte
diff --git a/database/rpcdb/db_client.go b/database/rpcdb/db_client.go
@@ -218,9 +218,9 @@ func (it *iterator) Next() bool {
 	resp, err := it.db.client.IteratorNext(context.Background(), &rpcdbproto.IteratorNextRequest{
 		Id: it.id,
 	})
+	it.errs.Add(updateError(err))
 
 	if err != nil {
-		it.errs.Add(err)
 		return false
 	}
 	it.key = resp.Key
@@ -252,7 +252,7 @@ func (it *iterator) Release() {
 	_, err := it.db.client.IteratorRelease(context.Background(), &rpcdbproto.IteratorReleaseRequest{
 		Id: it.id,
 	})
-	it.errs.Add(err)
+	it.errs.Add(updateError(err))
 }
 
 // updateError sets the error value to the errors required by the Database
diff --git a/database/test_database.go b/database/test_database.go
@@ -731,13 +731,17 @@ func TestStatNoPanic(t *testing.T, db Database) {
 		t.Fatalf("Unexpected error on batch.Put: %s", err)
 	}
 
-	db.Stat("") // #nosec G104
+	// Stat could error or not redpending on the implementation, but it
+	// shouldn't panic
+	_, _ = db.Stat("")
 
 	if err := db.Close(); err != nil {
 		t.Fatalf("Unexpected error on db.Close: %s", err)
 	}
 
-	db.Stat("") // #nosec G104
+	// Stat could error or not redpending on the implementation, but it
+	// shouldn't panic
+	_, _ = db.Stat("")
 }
 
 // TestCompactNoPanic ...
@@ -759,11 +763,15 @@ func TestCompactNoPanic(t *testing.T, db Database) {
 		t.Fatalf("Unexpected error on batch.Put: %s", err)
 	}
 
-	db.Compact(nil, nil) // #nosec G104
+	if err := db.Compact(nil, nil); err != nil {
+		t.Fatalf("Unexpected error on db.Compact")
+	}
 
 	if err := db.Close(); err != nil {
 		t.Fatalf("Unexpected error on db.Close: %s", err)
 	}
 
-	db.Compact(nil, nil) // #nosec G104
+	if err := db.Compact(nil, nil); err != ErrClosed {
+		t.Fatalf("Expected error %s on db.Close but got %s", ErrClosed, err)
+	}
 }
diff --git a/nat/nat.go b/nat/nat.go
@@ -103,7 +103,7 @@ func (dev *Mapper) keepPortMapping(mappedPort chan<- uint16, protocol string,
 
 				dev.log.Debug("Unmap protocol %s external port %d", protocol, extPort)
 				if err := dev.r.UnmapPort(protocol, intPort, extPort); err != nil {
-					dev.log.Error("Error unmapping port %d to %d: %w", intPort, extPort, err)
+					dev.log.Debug("Error unmapping port %d to %d: %s", intPort, extPort, err)
 				}
 
 				dev.wg.Done()
diff --git a/network/network.go b/network/network.go
@@ -228,9 +228,6 @@ func NewNetwork(
 	pingPongTimeout time.Duration,
 	pingFrequency time.Duration,
 ) Network {
-	// Ignore weak randomness warning because strong randomness is unnecessary for
-	// creating the nodeID
-	// #nosec G404
 	net := &network{
 		log:                                log,
 		id:                                 id,
@@ -269,7 +266,7 @@ func NewNetwork(
 		peers:           make(map[[20]byte]*peer),
 	}
 	if err := net.initialize(registerer); err != nil {
-		log.Error("error initializing network metrics: %w", err)
+		log.Warn("initializing network metrics failed with: %s", err)
 	}
 	net.executor.Initialize()
 	net.heartbeat()
@@ -404,7 +401,7 @@ func (n *network) Accepted(validatorID ids.ShortID, chainID ids.ID, requestID ui
 func (n *network) GetAncestors(validatorID ids.ShortID, chainID ids.ID, requestID uint32, containerID ids.ID) {
 	msg, err := n.b.GetAncestors(chainID, requestID, containerID)
 	if err != nil {
-		n.log.Error("failed to build GetAncestors message: %w", err)
+		n.log.Error("failed to build GetAncestors message: %s", err)
 		return
 	}
 
@@ -707,7 +704,7 @@ func (n *network) Close() error {
 	n.closed = true
 	err := n.listener.Close()
 	if err != nil {
-		n.log.Error("Error while closing network listener: %w", err)
+		n.log.Debug("closing network listener failed with: %s", err)
 	}
 
 	peersToClose := []*peer(nil)
diff --git a/snow/engine/snowman/bootstrapper.go b/snow/engine/snowman/bootstrapper.go
@@ -153,7 +153,7 @@ func (b *bootstrapper) MultiPut(vdr ids.ShortID, requestID uint32, blks [][]byte
 
 	wantedBlk, err := b.VM.ParseBlock(blks[0]) // the block we requested
 	if err != nil {
-		b.BootstrapConfig.Context.Log.Debug("Failed to parse requested block %s: %w", wantedBlkID, err)
+		b.BootstrapConfig.Context.Log.Debug("Failed to parse requested block %s: %s", wantedBlkID, err)
 		return b.fetch(wantedBlkID)
 	} else if actualID := wantedBlk.ID(); !actualID.Equals(wantedBlkID) {
 		b.BootstrapConfig.Context.Log.Debug("expected the first block to be the requested block, %s, but is %s", wantedBlk, actualID)
@@ -162,7 +162,7 @@ func (b *bootstrapper) MultiPut(vdr ids.ShortID, requestID uint32, blks [][]byte
 
 	for _, blkBytes := range blks {
 		if _, err := b.VM.ParseBlock(blkBytes); err != nil { // persists the block
-			b.BootstrapConfig.Context.Log.Debug("Failed to parse block: %w", err)
+			b.BootstrapConfig.Context.Log.Debug("Failed to parse block: %s", err)
 			b.BootstrapConfig.Context.Log.Verbo("block: %s", formatting.DumpBytes{Bytes: blkBytes})
 		}
 	}
diff --git a/snow/networking/router/handler.go b/snow/networking/router/handler.go
@@ -125,7 +125,7 @@ func (h *Handler) Initialize(
 ) {
 	h.ctx = engine.Context()
 	if err := h.metrics.Initialize(namespace, metrics); err != nil {
-		h.ctx.Log.Error("Error initializing handler metrics: %w", err)
+		h.ctx.Log.Warn("initializing handler metrics errored with: %s", err)
 	}
 	h.reliableMsgsSema = make(chan struct{}, 1)
 	h.closed = make(chan struct{})
diff --git a/vms/avm/vm.go b/vms/avm/vm.go
@@ -270,9 +270,7 @@ func (vm *VM) CreateStaticHandlers() map[string]*common.HTTPHandler {
 	newServer.RegisterCodec(codec, "application/json")
 	newServer.RegisterCodec(codec, "application/json;charset=UTF-8")
 	// name this service "avm"
-	if err := newServer.RegisterService(&StaticService{}, "avm"); err != nil {
-		vm.ctx.Log.Error("error creating static handlers: %w", err)
-	}
+	_ = newServer.RegisterService(&StaticService{}, "avm")
 	return map[string]*common.HTTPHandler{
 		"": {LockOptions: common.WriteLock, Handler: newServer},
 	}
diff --git a/vms/components/core/snowman_vm.go b/vms/components/core/snowman_vm.go
@@ -138,19 +138,19 @@ func (svm *SnowmanVM) NotifyBlockReady() {
 //   * The LockOption is the first element of [lockOption]
 //     By default the LockOption is WriteLock
 //     [lockOption] should have either 0 or 1 elements. Elements beside the first are ignored.
-func (svm *SnowmanVM) NewHandler(name string, service interface{}, lockOption ...common.LockOption) *common.HTTPHandler {
+func (svm *SnowmanVM) NewHandler(name string, service interface{}, lockOption ...common.LockOption) (*common.HTTPHandler, error) {
 	server := rpc.NewServer()
 	server.RegisterCodec(json.NewCodec(), "application/json")
 	server.RegisterCodec(json.NewCodec(), "application/json;charset=UTF-8")
 	if err := server.RegisterService(service, name); err != nil {
-		svm.Ctx.Log.Error("error creating static handlers: %w", err)
+		return nil, err
 	}
 
 	var lock common.LockOption = common.WriteLock
 	if len(lockOption) != 0 {
 		lock = lockOption[0]
 	}
-	return &common.HTTPHandler{LockOptions: lock, Handler: server}
+	return &common.HTTPHandler{LockOptions: lock, Handler: server}, nil
 }
 
 // Initialize this vm.
diff --git a/vms/manager.go b/vms/manager.go
@@ -120,7 +120,7 @@ func (m *manager) addStaticAPIEndpoints(vmID ids.ID) {
 		staticVM, ok := vm.(common.VM)
 		if ok {
 			if err := staticVM.Shutdown(); err != nil {
-				m.log.Error("Error while shutting down static API Endpoints: %w", err)
+				m.log.Error("shutting down static API endpoints errored with: %s", err)
 			}
 		}
 		return
diff --git a/vms/platformvm/service.go b/vms/platformvm/service.go
@@ -460,7 +460,7 @@ func (service *Service) ListAccounts(_ *http.Request, args *ListAccountsArgs, re
 	for _, accountID := range accountIDs {
 		account, err := service.vm.getAccount(service.vm.DB, accountID) // Get account whose ID is [accountID]
 		if err != nil && err != database.ErrNotFound {
-			service.vm.Ctx.Log.Error("couldn't get account from database: %w", err)
+			service.vm.Ctx.Log.Error("couldn't get account from database: %s", err)
 			continue
 		} else if err == database.ErrNotFound {
 			account = newAccount(accountID, 0, 0)
@@ -1388,7 +1388,7 @@ func (service *Service) CreateBlockchain(_ *http.Request, args *CreateBlockchain
 
 	txBytes, err := Codec.Marshal(genericTx{Tx: &tx})
 	if err != nil {
-		service.vm.Ctx.Log.Error("problem marshaling createChainTx: %w", err)
+		service.vm.Ctx.Log.Error("problem marshaling createChainTx: %s", err)
 		return errCreatingTransaction
 	}
 
diff --git a/vms/platformvm/vm.go b/vms/platformvm/vm.go
@@ -631,14 +631,15 @@ func (vm *VM) SetPreference(blkID ids.ID) {
 // See API documentation for more information
 func (vm *VM) CreateHandlers() map[string]*common.HTTPHandler {
 	// Create a service with name "platform"
-	handler := vm.SnowmanVM.NewHandler("platform", &Service{vm: vm})
+	handler, err := vm.SnowmanVM.NewHandler("platform", &Service{vm: vm})
+	vm.Ctx.Log.AssertNoError(err)
 	return map[string]*common.HTTPHandler{"": handler}
 }
 
 // CreateStaticHandlers implements the snowman.ChainVM interface
 func (vm *VM) CreateStaticHandlers() map[string]*common.HTTPHandler {
 	// Static service's name is platform
-	handler := vm.SnowmanVM.NewHandler("platform", &StaticService{})
+	handler, _ := vm.SnowmanVM.NewHandler("platform", &StaticService{})
 	return map[string]*common.HTTPHandler{
 		"": handler,
 	}
diff --git a/vms/spchainvm/vm.go b/vms/spchainvm/vm.go
@@ -224,9 +224,7 @@ func (vm *VM) CreateStaticHandlers() map[string]*common.HTTPHandler {
 	newServer.RegisterCodec(codec, "application/json")
 	newServer.RegisterCodec(codec, "application/json;charset=UTF-8")
 	// Name the API service "spchain"
-	if err := newServer.RegisterService(&StaticService{}, "spchain"); err != nil {
-		vm.ctx.Log.Error("error creating static handlers: %w", err)
-	}
+	_ = newServer.RegisterService(&StaticService{}, "spchain")
 	return map[string]*common.HTTPHandler{
 		"": &common.HTTPHandler{LockOptions: common.NoLock, Handler: newServer},
 	}
diff --git a/vms/spdagvm/vm.go b/vms/spdagvm/vm.go
@@ -169,9 +169,7 @@ func (vm *VM) CreateStaticHandlers() map[string]*common.HTTPHandler {
 	newServer.RegisterCodec(codec, "application/json")
 	newServer.RegisterCodec(codec, "application/json;charset=UTF-8")
 	// name this service "spdag"
-	if err := newServer.RegisterService(&StaticService{}, "spdag"); err != nil {
-		vm.ctx.Log.Error("error creating static handlers: %w", err)
-	}
+	_ = newServer.RegisterService(&StaticService{}, "spdag")
 	return map[string]*common.HTTPHandler{
 		// NoLock because the static functions probably wont be stateful (i.e. no
 		// write operations)
diff --git a/vms/timestampvm/vm.go b/vms/timestampvm/vm.go
@@ -100,7 +100,8 @@ func (vm *VM) Initialize(
 // Keys: The path extension for this VM's API (empty in this case)
 // Values: The handler for the API
 func (vm *VM) CreateHandlers() map[string]*common.HTTPHandler {
-	handler := vm.NewHandler("timestamp", &Service{vm})
+	handler, err := vm.NewHandler("timestamp", &Service{vm})
+	vm.Ctx.Log.AssertNoError(err)
 	return map[string]*common.HTTPHandler{
 		"": handler,
 	}

Original file line number	Diff line number	Diff line change
`@@ -89,9 +89,7 @@ func (ipc IPCs) PublishBlockchain(r http.Request, args *PublishBlockchainArgs,`
`89`	`89`
`90`	`90`	`if err = sock.Listen(url); err != nil {`
`91`	`91`	`ipc.log.Error("can't listen on pub socket: %s", err)`
`92`		`- // Closing the socket should only error if it has already been closed`
`93`		`- // #nosec G104`
`94`		`- sock.Close()`
	`92`	`+ _ = sock.Close() // Return the original error`
`95`	`93`	`return err`
`96`	`94`	`}`
`97`	`95`
`@@ -101,9 +99,7 @@ func (ipc IPCs) PublishBlockchain(r http.Request, args *PublishBlockchainArgs,`
`101`	`99`	`}`
`102`	`100`	`if err := ipc.events.RegisterChain(chainID, "ipc", chainIPC); err != nil {`
`103`	`101`	`ipc.log.Error("couldn't register event: %s", err)`
`104`		`- // Closing the socket should only error if it has already been closed`
`105`		`- // #nosec G104`
`106`		`- sock.Close()`
	`102`	`+ _ = sock.Close() // Return the original error`
`107`	`103`	`return err`
`108`	`104`	`}`
`109`	`105`
Original file line number	Diff line number	Diff line change
`@@ -379,7 +379,7 @@ func (m manager) buildChain(chainParams ChainParameters) (chain, error) {`
`379`	`379`	`ctx.Lock.Lock()`
`380`	`380`	`defer ctx.Lock.Unlock()`
`381`	`381`	`if err := chain.Engine.Startup(); err != nil {`
`382`		`- chain.Ctx.Log.Error("failed to start consensus engine: %w", err)`
	`382`	`+ chain.Ctx.Log.Error("failed to start consensus engine: %s", err)`
`383`	`383`	`chain.Handler.Shutdown()`
`384`	`384`	`}`
`385`	`385`	`})`
Original file line number	Diff line number	Diff line change
`@@ -731,13 +731,17 @@ func TestStatNoPanic(t *testing.T, db Database) {`
`731`	`731`	`t.Fatalf("Unexpected error on batch.Put: %s", err)`
`732`	`732`	`}`
`733`	`733`
`734`		`- db.Stat("") // #nosec G104`
	`734`	`+ // Stat could error or not redpending on the implementation, but it`
	`735`	`+ // shouldn't panic`
	`736`	`+ _, _ = db.Stat("")`
`735`	`737`
`736`	`738`	`if err := db.Close(); err != nil {`
`737`	`739`	`t.Fatalf("Unexpected error on db.Close: %s", err)`
`738`	`740`	`}`
`739`	`741`
`740`		`- db.Stat("") // #nosec G104`
	`742`	`+ // Stat could error or not redpending on the implementation, but it`
	`743`	`+ // shouldn't panic`
	`744`	`+ _, _ = db.Stat("")`
`741`	`745`	`}`
`742`	`746`
`743`	`747`	`// TestCompactNoPanic ...`
`@@ -759,11 +763,15 @@ func TestCompactNoPanic(t *testing.T, db Database) {`
`759`	`763`	`t.Fatalf("Unexpected error on batch.Put: %s", err)`
`760`	`764`	`}`
`761`	`765`
`762`		`- db.Compact(nil, nil) // #nosec G104`
	`766`	`+ if err := db.Compact(nil, nil); err != nil {`
	`767`	`+ t.Fatalf("Unexpected error on db.Compact")`
	`768`	`+ }`
`763`	`769`
`764`	`770`	`if err := db.Close(); err != nil {`
`765`	`771`	`t.Fatalf("Unexpected error on db.Close: %s", err)`
`766`	`772`	`}`
`767`	`773`
`768`		`- db.Compact(nil, nil) // #nosec G104`
	`774`	`+ if err := db.Compact(nil, nil); err != ErrClosed {`
	`775`	`+ t.Fatalf("Expected error %s on db.Close but got %s", ErrClosed, err)`
	`776`	`+ }`
`769`	`777`	`}`
Original file line number	Diff line number	Diff line change
`@@ -103,7 +103,7 @@ func (dev *Mapper) keepPortMapping(mappedPort chan<- uint16, protocol string,`
`103`	`103`
`104`	`104`	`dev.log.Debug("Unmap protocol %s external port %d", protocol, extPort)`
`105`	`105`	`if err := dev.r.UnmapPort(protocol, intPort, extPort); err != nil {`
`106`		`- dev.log.Error("Error unmapping port %d to %d: %w", intPort, extPort, err)`
	`106`	`+ dev.log.Debug("Error unmapping port %d to %d: %s", intPort, extPort, err)`
`107`	`107`	`}`
`108`	`108`
`109`	`109`	`dev.wg.Done()`
Original file line number	Diff line number	Diff line change
`@@ -125,7 +125,7 @@ func (h *Handler) Initialize(`
`125`	`125`	`) {`
`126`	`126`	`h.ctx = engine.Context()`
`127`	`127`	`if err := h.metrics.Initialize(namespace, metrics); err != nil {`
`128`		`- h.ctx.Log.Error("Error initializing handler metrics: %w", err)`
	`128`	`+ h.ctx.Log.Warn("initializing handler metrics errored with: %s", err)`
`129`	`129`	`}`
`130`	`130`	`h.reliableMsgsSema = make(chan struct{}, 1)`
`131`	`131`	`h.closed = make(chan struct{})`