Improve Pacer Error Handling #1938
Conversation
throughput/pacer.go
Outdated
| } | ||
|
|
||
| func (p *pacer) Run(ctx context.Context, max int) { | ||
| p.inflight = make(chan struct{}, max) | ||
| p.done = make(chan error, 1) | ||
| p.done = make(chan struct{}, 1) |
There was a problem hiding this comment.
Can this be empty? We only need to signal it's been closed, there's no need to initialize it to a length of 1
throughput/pacer.go
Outdated
| if !ok { | ||
| return | ||
| } |
There was a problem hiding this comment.
Why do we need this !ok check?
throughput/spam.go
Outdated
| @@ -341,6 +341,7 @@ func (s *Spammer) distributeFunds(ctx context.Context, parser chain.Parser, feeP | |||
| return nil, nil, err | |||
| } | |||
| if err := p.Add(tx); err != nil { | |||
| _ = p.Wait() | |||
There was a problem hiding this comment.
Why do we need to call p.Wait() here? Same comment below.
| if p.err != nil { | ||
| return p.err | ||
| } | ||
|
|
||
| if err := p.ws.RegisterTx(tx); err != nil { | ||
| return err | ||
| p.setError(err) | ||
| return p.err |
There was a problem hiding this comment.
Do we need to check p.err here or can we register the tx and read the err iff p.done has been closed?
Since this value is set at most once immediately before p.done is closed, we should technically check that the channel has been closed before reading this value.
| func (p *pacer) setError(err error) { | ||
| p.errOnce.Do(func() { | ||
| p.err = err | ||
| close(p.done) | ||
| }) |
There was a problem hiding this comment.
Can we make this even simpler by removing errOnce and using defer close(p.done) within Run ?
This way there's only one place where p.done can be closed and it's explicit that it will be called once from that function.
If we handle it this way, it means that we can write the error value once within that function and rely on the deferred call to make sure it's closed before any reader accesses it (assuming we also remove reads of p.err within Add.
| if !ok { | ||
| p.setError(nil) | ||
| return | ||
| } |
There was a problem hiding this comment.
Why do we need this check?
This PR improves the error handling of
pacerwith the following:setError()method to make sure errors are written exactly once.errorwhich allows for one write, multiple readsRun()to terminate if an error was produced inAdd()Add()to terminate if an error was caught and to stop pacer if WS client produces an error.Wait()to wait for outstanding TXs to be processed and to set a nil error if no error was caught.