Skip to content

Commit

Permalink
PR Feedback.
Browse files Browse the repository at this point in the history
  • Loading branch information
@winder
winder committed Nov 20, 2021
1 parent b7bf5d1 commit cd7c063
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 38 deletions.
58 changes: 25 additions & 33 deletions api/handlers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ import (
log "github.com/sirupsen/logrus"

"github.com/algorand/go-algorand/data/basics"
"github.com/algorand/go-algorand/data/bookkeeping"

"github.com/algorand/indexer/accounting"
"github.com/algorand/indexer/api/generated/common"
"github.com/algorand/indexer/api/generated/v2"
Expand Down Expand Up @@ -118,7 +116,7 @@ func (si *ServerImplementation) MakeHealthCheck(ctx echo.Context) error {
return err
})
if err != nil {
return indexerError(ctx, err, fmt.Sprintf("%s: %v", errFailedLookingUpHealth, err))
return indexerError(ctx, fmt.Errorf("%s: %w", errFailedLookingUpHealth, err))
}

if health.Error != "" {
Expand Down Expand Up @@ -158,15 +156,15 @@ func (si *ServerImplementation) LookupAccountByID(ctx echo.Context, accountID st

accounts, round, err := si.fetchAccounts(ctx.Request().Context(), options, params.Round)
if err != nil {
return indexerError(ctx, err, fmt.Sprintf("%s: %v", errFailedSearchingAccount, err))
return indexerError(ctx, fmt.Errorf("%s: %w", errFailedSearchingAccount, err))
}

if len(accounts) == 0 {
return notFound(ctx, fmt.Sprintf("%s: %s", errNoAccountsFound, accountID))
}

if len(accounts) > 1 {
return indexerError(ctx, nil, fmt.Sprintf("%s: %s", errMultipleAccounts, accountID))
return indexerError(ctx, fmt.Errorf("%s: %s", errMultipleAccounts, accountID))
}

return ctx.JSON(http.StatusOK, generated.AccountResponse{
Expand Down Expand Up @@ -217,7 +215,7 @@ func (si *ServerImplementation) SearchForAccounts(ctx echo.Context, params gener
accounts, round, err := si.fetchAccounts(ctx.Request().Context(), options, params.Round)

if err != nil {
return indexerError(ctx, err, fmt.Sprintf("%s: %v", errFailedSearchingAccount, err))
return indexerError(ctx, fmt.Errorf("%s: %w", errFailedSearchingAccount, err))
}

var next *string
Expand Down Expand Up @@ -274,7 +272,7 @@ func (si *ServerImplementation) LookupAccountTransactions(ctx echo.Context, acco
func (si *ServerImplementation) SearchForApplications(ctx echo.Context, params generated.SearchForApplicationsParams) error {
apps, round, err := si.fetchApplications(ctx.Request().Context(), params)
if err != nil {
return indexerError(ctx, err, fmt.Sprintf("%s: %v", errFailedSearchingApplication, err))
return indexerError(ctx, fmt.Errorf("%s: %w", errFailedSearchingApplication, err))
}

var next *string
Expand All @@ -300,15 +298,15 @@ func (si *ServerImplementation) LookupApplicationByID(ctx echo.Context, applicat

apps, round, err := si.fetchApplications(ctx.Request().Context(), p)
if err != nil {
return indexerError(ctx, err, fmt.Sprintf("%s: %v", errFailedSearchingApplication, err))
return indexerError(ctx, fmt.Errorf("%s: %w", errFailedSearchingApplication, err))
}

if len(apps) == 0 {
return notFound(ctx, fmt.Sprintf("%s: %d", errNoApplicationsFound, applicationID))
}

if len(apps) > 1 {
return indexerError(ctx, nil, fmt.Sprintf("%s: %d", errMultipleApplications, applicationID))
return indexerError(ctx, fmt.Errorf("%s: %d", errMultipleApplications, applicationID))
}

return ctx.JSON(http.StatusOK, generated.ApplicationResponse{
Expand Down Expand Up @@ -345,7 +343,7 @@ func (si *ServerImplementation) LookupApplicationLogsByID(ctx echo.Context, appl
// Fetch the transactions
txns, next, round, err := si.fetchTransactions(ctx.Request().Context(), filter)
if err != nil {
return indexerError(ctx, err, fmt.Sprintf("%s: %v", errTransactionSearch, err))
return indexerError(ctx, fmt.Errorf("%s: %w", errTransactionSearch, err))
}

var logData []generated.ApplicationLogData
Expand Down Expand Up @@ -388,15 +386,15 @@ func (si *ServerImplementation) LookupAssetByID(ctx echo.Context, assetID uint64

assets, round, err := si.fetchAssets(ctx.Request().Context(), options)
if err != nil {
return indexerError(ctx, err, fmt.Sprintf("%s: %v", errFailedSearchingAsset, err))
return indexerError(ctx, fmt.Errorf("%s: %w", errFailedSearchingAsset, err))
}

if len(assets) == 0 {
return notFound(ctx, fmt.Sprintf("%s: %d", errNoAssetsFound, assetID))
}

if len(assets) > 1 {
return indexerError(ctx, nil, fmt.Sprintf("%s: %d", errMultipleAssets, assetID))
return indexerError(ctx, fmt.Errorf("%s: %d", errMultipleAssets, assetID))
}

return ctx.JSON(http.StatusOK, generated.AssetResponse{
Expand Down Expand Up @@ -426,7 +424,7 @@ func (si *ServerImplementation) LookupAssetBalances(ctx echo.Context, assetID ui

balances, round, err := si.fetchAssetBalances(ctx.Request().Context(), query)
if err != nil {
return indexerError(ctx, err, fmt.Sprintf("%s: %v", errFailedSearchingAssetBalances, err))
return indexerError(ctx, fmt.Errorf("%s: %w", errFailedSearchingAssetBalances, err))
}

var next *string
Expand Down Expand Up @@ -479,7 +477,7 @@ func (si *ServerImplementation) SearchForAssets(ctx echo.Context, params generat

assets, round, err := si.fetchAssets(ctx.Request().Context(), options)
if err != nil {
return indexerError(ctx, err, fmt.Sprintf("%s: %v", errFailedSearchingAsset, err))
return indexerError(ctx, fmt.Errorf("%s: %w", errFailedSearchingAsset, err))
}

var next *string
Expand All @@ -502,7 +500,7 @@ func (si *ServerImplementation) LookupBlock(ctx echo.Context, roundNumber uint64
return notFound(ctx, fmt.Sprintf("%s '%d': %v", errLookingUpBlock, roundNumber, err))
}
if err != nil {
return indexerError(ctx, err, fmt.Sprintf("%s '%d': %v", errLookingUpBlock, roundNumber, err))
return indexerError(ctx, fmt.Errorf("%s '%d': %w", errLookingUpBlock, roundNumber, err))
}

return ctx.JSON(http.StatusOK, generated.BlockResponse(blk))
Expand All @@ -525,15 +523,15 @@ func (si *ServerImplementation) LookupTransaction(ctx echo.Context, txid string)
// Fetch the transactions
txns, _, round, err := si.fetchTransactions(ctx.Request().Context(), filter)
if err != nil {
return indexerError(ctx, err, fmt.Sprintf("%s: %v", errTransactionSearch, err))
return indexerError(ctx, fmt.Errorf("%s: %w", errTransactionSearch, err))
}

if len(txns) == 0 {
return notFound(ctx, fmt.Sprintf("%s: %s", errNoTransactionFound, txid))
}

if len(txns) > 1 {
return indexerError(ctx, nil, fmt.Sprintf("%s: %s", errMultipleTransactions, txid))
return indexerError(ctx, fmt.Errorf("%s: %s", errMultipleTransactions, txid))
}

response := generated.TransactionResponse{
Expand All @@ -560,7 +558,7 @@ func (si *ServerImplementation) SearchForTransactions(ctx echo.Context, params g
// Fetch the transactions
txns, next, round, err := si.fetchTransactions(ctx.Request().Context(), filter)
if err != nil {
return indexerError(ctx, err, fmt.Sprintf("%s: %v", errTransactionSearch, err))
return indexerError(ctx, fmt.Errorf("%s: %w", errTransactionSearch, err))
}

response := generated.TransactionsResponse{
Expand Down Expand Up @@ -591,16 +589,13 @@ func timeoutError(ctx echo.Context, err string) error {
}

// return a 500, or 503 if it is a timeout error
func indexerError(ctx echo.Context, err error, msg string) error {
var code int
func indexerError(ctx echo.Context, err error) error {
if util.IsTimeoutError(err) {
code = http.StatusServiceUnavailable
} else {
code = http.StatusInternalServerError
return timeoutError(ctx, err.Error())
}

return ctx.JSON(code, generated.ErrorResponse{
Message: msg,
return ctx.JSON(http.StatusInternalServerError, generated.ErrorResponse{
Message: err.Error(),
})
}

Expand Down Expand Up @@ -753,13 +748,9 @@ func (si *ServerImplementation) fetchAssetBalances(ctx context.Context, options
// fetchBlock looks up a block and converts it into a generated.Block object
// the method also loads the transactions into the returned block object.
func (si *ServerImplementation) fetchBlock(ctx context.Context, round uint64) (generated.Block, error) {
var blockHeader bookkeeping.BlockHeader
var transactions []idb.TxnRow
var ret generated.Block
results := make([]generated.Transaction, 0)
var err error
err = util.CallWithTimeout(ctx, si.log, si.timeout, func(ctx context.Context) error {
blockHeader, transactions, err =
err := util.CallWithTimeout(ctx, si.log, si.timeout, func(ctx context.Context) error {
blockHeader, transactions, err :=
si.db.GetBlock(ctx, round, idb.GetBlockOptions{Transactions: true})
if err != nil {
return err
Expand Down Expand Up @@ -803,6 +794,7 @@ func (si *ServerImplementation) fetchBlock(ctx context.Context, round uint64) (g
UpgradeVote: &upgradeVote,
}

results := make([]generated.Transaction, 0)
for _, txrow := range transactions {
tx, err := txnRowToTransaction(txrow)
if err != nil {
Expand All @@ -823,10 +815,10 @@ func (si *ServerImplementation) fetchBlock(ctx context.Context, round uint64) (g
// fetchAccounts queries for accounts and converts them into generated.Account
// objects, optionally rewinding their value back to a particular round.
func (si *ServerImplementation) fetchAccounts(ctx context.Context, options idb.AccountQueryOptions, atRound *uint64) ([]generated.Account, uint64 /*round*/, error) {
var accountchan <-chan idb.AccountRow
var round uint64
accounts := make([]generated.Account, 0)
err := util.CallWithTimeout(ctx, si.log, si.timeout, func(ctx context.Context) error {
var accountchan <-chan idb.AccountRow
accountchan, round = si.db.GetAccounts(ctx, options)

if (atRound != nil) && (*atRound > round) {
Expand Down Expand Up @@ -871,12 +863,12 @@ func (si *ServerImplementation) fetchAccounts(ctx context.Context, options idb.A

// fetchTransactions is used to query the backend for transactions, and compute the next token
func (si *ServerImplementation) fetchTransactions(ctx context.Context, filter idb.TransactionFilter) ([]generated.Transaction, string, uint64 /*round*/, error) {
var txchan <-chan idb.TxnRow
var round uint64
var nextToken string
var err error
results := make([]generated.Transaction, 0)
err = util.CallWithTimeout(ctx, si.log, si.timeout, func(ctx context.Context) error {
var txchan <-chan idb.TxnRow
txchan, round = si.db.Transactions(ctx, filter)

rootTxnDedupeMap := make(map[string]struct{})
Expand Down
14 changes: 10 additions & 4 deletions util/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ func IsTimeoutError(err error) bool {
return errors.Is(err, ErrTimeout)
}

// ErrMisbehavingHandler is written to the log when a handler does not return.
var ErrMisbehavingHandler = "Misbehaving handler did not exist after 1 second."
// errMisbehavingHandler is written to the log when a handler does not return.
var errMisbehavingHandler = "Misbehaving handler did not exist after 1 second."

// misbehavingHandlerDetector warn if ch does not exit after 1 second.
func misbehavingHandlerDetector(log *log.Logger, ch chan struct{}) {
Expand All @@ -40,7 +40,7 @@ func misbehavingHandlerDetector(log *log.Logger, ch chan struct{}) {
// Good. This means the handler returns shortly after the context finished.
return
case <-time.After(1 * time.Second):
log.Warnf(ErrMisbehavingHandler)
log.Warnf(errMisbehavingHandler)
}
}

Expand All @@ -53,7 +53,7 @@ func CallWithTimeout(ctx context.Context, log *log.Logger, timeout time.Duration
return handler(ctx)
}

timeoutCtx, cancel := context.WithTimeout(context.Background(), timeout)
timeoutCtx, cancel := context.WithTimeout(ctx, timeout)
defer cancel()

// Call function in go routine
Expand All @@ -67,6 +67,12 @@ func CallWithTimeout(ctx context.Context, log *log.Logger, timeout time.Duration
// wait for task to finish or context to timeout/cancel
select {
case <-done:
// This may not be possible, but in theory the handler would quickly terminate
// when the context deadline is reached. So make sure the handler didn't finish
// due to a timeout.
if timeoutCtx.Err() == context.DeadlineExceeded {
return ErrTimeout
}
return err
case <-timeoutCtx.Done():
go misbehavingHandlerDetector(log, done)
Expand Down
2 changes: 1 addition & 1 deletion util/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func TestCallWithTimeout_timeout(t *testing.T) {

time.Sleep(2 * time.Second)
require.Len(t, hook.Entries, 1)
require.Equal(t, ErrMisbehavingHandler, hook.LastEntry().Message)
require.Equal(t, errMisbehavingHandler, hook.LastEntry().Message)
}

func TestCallWithTimeout_noTimeout(t *testing.T) {
Expand Down

0 comments on commit cd7c063

Please sign in to comment.