query: add OnMaxTries #308
Conversation
OnMaxTries config lets the caller choose what happens when a query goes over the maximum allowed tries. This config helps meet the needs of different callers as some may choose to immediately disconnect vs others that may choose to try again.
The peer may already be disconnected but the go runtime could have chosen to handle the message. To ensure that the handleResponse doesn't get called when the peer is already disconnected, add a select before handleResponse.
|
cc: @mohamedawnallah for review |
| @@ -94,6 +94,10 @@ type Config struct { | |||
| // make this configurable to easily mock the worker used during tests. | |||
| NewWorker func(Peer) Worker | |||
|
|
|||
| // OnMaxTries is function closure that's called on max retries on | |||
| // workers. | |||
| OnMaxTries func(string) | |||
There was a problem hiding this comment.
think we should instead use a stricter type here (net.Addr) and mention what is being passed to this call-back so that the caller doesnt have to go look at the code to figure out what is being passed to the call-back it is setting.
The commit message isnt very accurate imo. OnMaxTries config lets the caller choose what happens when a query goes over the maximum allowed tries isnt really true - more so: OnMaxTries gives the caller access to the peer's address once a maxmum number of retries have been attempted" or something like that
There was a problem hiding this comment.
it's worth linking the PR that makes use of this new config option in the PR description so that reviewers can understand the context a bit more
There was a problem hiding this comment.
think we should instead use a stricter type here (net.Addr) and mention what is being passed to this call-back so that the caller doesnt have to go look at the code to figure out what is being passed to the call-back it is setting.
The commit message isnt very accurate imo.
OnMaxTries config lets the caller choose what happens when a query goes over the maximum allowed triesisnt really true - more so:OnMaxTriesgives the caller access to the peer's address once a maxmum number of retries have been attempted" or something like that
Will address both points.
| select { | ||
| // If the peer disconnects before giving us a valid | ||
| // answer, we'll also exit with an error. | ||
| case <-peer.OnDisconnect(): | ||
| log.Debugf("Peer %v for worker disconnected, "+ | ||
| "cancelling job %v", peer.Addr(), | ||
| job.Index()) | ||
|
|
||
| jobErr = ErrPeerDisconnected | ||
| break Loop |
There was a problem hiding this comment.
i dont think this adds anything. This select is already covered in below and so if peer.OnDisconnect() triggers before the message is received on the msgChan, we will already catch the case below.
I think what you instead want is for job.HandleResp's function to know if it can exit or not.
The idiomatic way of doing this would be to pass it a ctx context.Context which gets cancelled when the peer disconnects. An intermediate step can just be to pass it a quit chan struct{} which you can derive from the peers OnDisconnect method and then listen on that in the HandleResp call-back
There was a problem hiding this comment.
So the reason for adding this is because I saw that there are cases where case resp := <- msgChan: gets triggered first before the below case <-peer.OnDisconnect(): gets triggered.
I think what you instead want is for job.HandleResp's function to know if it can exit or not.
Yeah that would also solve the problem I was seeing.
An intermediate step can just be to pass it a quit chan struct{} which you can derive from the peers OnDisconnect method
That would be ok as well except now we force every implementation of HandleResp to handle the case where quit chan struct{} is passed.
There was a problem hiding this comment.
Well, I'll also add that I don't really have a preference on how that's handled so I'll push changes so that HandleResp takes a quti chan struct{} as an argument.
|
@kcalvinalvin, remember to re-request review from reviewers when ready |
OnMaxTries allows an outside caller to dictate what should happen to a given worker in case of a timeout.
This allows for peer selection strategies to be made outside of the package, allowing for greater flexibility.
EDIT:
This PR is a dependency for btcsuite/btcd#2226