Skip to content

Handle 429 from accountant #2101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vkuznecovas merged 1 commit into from
Apr 23, 2020
Merged

Handle 429 from accountant #2101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 43 additions & 10 deletions session/pingpong/accountant_caller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package pingpong

import (
"context"
"encoding/hex"
"encoding/json"
"errors"
Expand All @@ -27,6 +28,7 @@ import (
"strings"
"time"

"github.com/cenkalti/backoff/v4"
"github.com/ethereum/go-ethereum/common"
"github.com/mysteriumnetwork/node/requests"
"github.com/mysteriumnetwork/payments/crypto"
Expand Down Expand Up @@ -119,13 +121,27 @@ func (ac *AccountantCaller) RequestPromise(rp RequestPromise) (crypto.Promise, e
return crypto.Promise{}, fmt.Errorf("could not form request_promise request: %w", err)
}

eback := backoff.NewConstantBackOff(time.Millisecond * 500)
boff := backoff.WithMaxRetries(eback, 3)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
boff = backoff.WithContext(boff, ctx)

res := crypto.Promise{}
err = ac.doRequest(req, &res)
if err != nil {
return res, fmt.Errorf("could not request promise: %w", err)

}
return res, nil
return res, backoff.Retry(func() error {
err = ac.doRequest(req, &res)
if err != nil {
// if too many requests, retry

@anjmao anjmao Apr 22, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure that after 1.5 second lock will be released? Maybe it would be safer and more clear to block in endpoint so it waits until lock is freed inside hermes (the same as simple locks works). In case it can't finish (let's say lock is not freed for some reason) this request will timeout.

@vkuznecovas vkuznecovas Apr 23, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indented to TRY again, not to ensure success. The accountant will lock for very short periods of time(less than 300ms currently). Besides, this error is not critical at all, as you can skip a few reveals/requests just fine.

if errors.Is(err, ErrTooManyRequests) {
return err
}
// otherwise, do not retry anymore and return the error
cancel()
return fmt.Errorf("could not request promise: %w", err)
}
return nil
}, boff)
}

// RevealObject represents the reveal request object.
Expand All @@ -145,12 +161,25 @@ func (ac *AccountantCaller) RevealR(r, provider string, agreementID uint64) erro
if err != nil {
return fmt.Errorf("could not form reveal_r request: %w", err)
}
err = ac.doRequest(req, &RevealSuccess{})
if err != nil {
return fmt.Errorf("could not reveal R for accountant: %w", err)
}

return nil
eback := backoff.NewConstantBackOff(time.Millisecond * 500)
boff := backoff.WithMaxRetries(eback, 3)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
boff = backoff.WithContext(boff, ctx)
return backoff.Retry(func() error {
err = ac.doRequest(req, &RevealSuccess{})
if err != nil {
// if too many requests, retry
if errors.Is(err, ErrTooManyRequests) {
return err
}
// otherwise, do not retry anymore and return the error
cancel()
return fmt.Errorf("could not reveal R for accountant: %w", err)
}
return nil
}, boff)
}

// GetConsumerData gets consumer data from accountant
Expand Down Expand Up @@ -303,6 +332,9 @@ var ErrAccountantHashlockMissmatch = errors.New("hashlock missmatch")
// ErrAccountantNotFound occurs when a requested resource is not found
var ErrAccountantNotFound = errors.New("resource not found")

// ErrTooManyRequests occurs when we call the reveal R or request promise errors asynchronously at the same time.
var ErrTooManyRequests = errors.New("too many simultaneous requests")

var accountantCauseToError = map[string]error{
ErrAccountantInvalidSignature.Error(): ErrAccountantInvalidSignature,
ErrAccountantInternal.Error(): ErrAccountantInternal,
Expand All @@ -316,6 +348,7 @@ var accountantCauseToError = map[string]error{
ErrAccountantHashlockMissmatch.Error(): ErrAccountantHashlockMissmatch,
ErrAccountantNotFound.Error(): ErrAccountantNotFound,
ErrNeedsRRecovery.Error(): ErrNeedsRRecovery,
ErrTooManyRequests.Error(): ErrTooManyRequests,
}

type rRecoveryDetails struct {
Expand Down
3 changes: 2 additions & 1 deletion session/pingpong/invoice_tracker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,8 @@ func (it *InvoiceTracker) handleAccountantError(err error) error {
case
stdErr.Is(err, ErrAccountantInternal),
stdErr.Is(err, ErrAccountantNotFound),
stdErr.Is(err, ErrAccountantMalformedJSON):
stdErr.Is(err, ErrAccountantMalformedJSON),
stdErr.Is(err, ErrTooManyRequests):
// these are ignorable, we'll eventually fail
if it.incrementAccountantFailureCount() > it.deps.MaxAccountantFailureCount {
return err
Expand Down