search

INFURA / go-ethlibs

Ethereum libraries in Go for interacting with Ethereum nodes
MIT License
162 stars 34 forks source link

Make sure we return the correct ctx.Err #32

Closed ryanschneider closed 4 years ago

ryanschneider commented 4 years ago

I just ran into a corner case during a network issue when debugging locally that I'm pretty sure I traced back to this issue.

Normally loopingTransport.ctx and the ctx passed into .Request are the same, or at least in the same tree, but they don't have to be, so it's definitely a bug that we'd do <-t.ctx.Done but return ctx.Err(). This led to the scenario where .Request() returned (nil, nil) which it shouldn't.

JeeChoi commented 4 years ago

question/FYI: do we know if t.ctx.Err() != nil? cause errors.Wrap(nil, string) returns nil

https://github.com/pkg/errors/issues/90

ryanschneider commented 4 years ago

question/FYI: do we know if t.ctx.Err() != nil? cause errors.Wrap(nil, string) returns nil

pkg/errors#90

Ya, it returns nil. This is so you can do things like: return errors.Wrap(someOperation(), "operation failed") I agree with the discussion in your link that MaybeWrap would've been a better choice for this behavior but it's how pkg/errors has always behaved as far as I know.