Skip to content

Instantly share code, notes, and snippets.

@dnldd
Last active October 31, 2018 23:45
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save dnldd/68a0a0d880a44bba7c3dfeb7b3f65255 to your computer and use it in GitHub Desktop.
Save dnldd/68a0a0d880a44bba7c3dfeb7b3f65255 to your computer and use it in GitHub Desktop.
1. The if-else here https://github.com/raedahgroup/dcrtxmatcher/blob/master/dcrtxmatcher.go#L43,
https://github.com/raedahgroup/dcrtxmatcher/blob/master/dcrtxmatcher.go#L82 would be more readable
broken into two if statements with comments explaining what constitutes a blind server and a
normal server.
2. uneeded newlines here https://github.com/raedahgroup/dcrtxmatcher/blob/master/dcrtxmatcher.go#L55,
https://github.com/raedahgroup/dcrtxmatcher/blob/master/dcrtxmatcher.go#L71,
https://github.com/raedahgroup/dcrtxmatcher/blob/master/dcrtxmatcher.go#L32,
https://github.com/raedahgroup/dcrtxmatcher/blob/master/service.go#L33,
https://github.com/raedahgroup/dcrtxmatcher/blob/master/service.go#L77
3. You have this snippet handling repeated quite a bit:
if done(ctx) {
return ctx.Err()
}
here: https://github.com/raedahgroup/dcrtxmatcher/blob/master/dcrtxmatcher.go#L101-L103,
https://github.com/raedahgroup/dcrtxmatcher/blob/master/dcrtxmatcher.go#L39-L41,
https://github.com/raedahgroup/dcrtxmatcher/blob/master/dcrtxmatcher.go#L109-L111
better off using a non blocking receive for the context in a loop, after starting
the server's listen and serve goroutine.
out:
for {
select {
case <-ctx.Done():
break out
default:
// Non-blocking receive fallthrough.
}
}
4. Unsure what you need this for: https://github.com/raedahgroup/dcrtxmatcher/blob/master/dcrtxmatcher.go#L139
do elaborate.
5. The grammar of comments need improvement, please refer to dicemix's review and update accordingly.
6. I think FindMatches can be simplified, does AddParticipant really need a goroutine? https://github.com/raedahgroup/dcrtxmatcher/blob/master/service.go#L32
7. I get the pattern you're going for with SplitTxMatcherService methods, you want the cancellation signal to terminate every executing process when given. Still giving it a thought,
you might be overdoing it I'm not entirely sure yet, will get back to you on this.
*Update*, discussed this with Dave:
```
If you cancel the context after the long running queue join has began, the code will leak the goroutine
The goroutine will be blocked on sending to the done channel which nothing is listening for anymore because it was cancelled
Need to buffer it So that when the cancel happens, the sending gr will still send the data into the buffered channel and then it will go out of scope and get garbage collected.
Also, it looks `peer, _ := peer.FromContext(ctx)` is a *gross* misuse of context
https://medium.com/@cep21/how-to-correctly-use-context-context-in-go-1-7-8f2c0fafdf39
And storing the peer in the context is horrible `Context.Value should inform, not control`
That `peer, _ := peer.FromContext(ctx)` is clearly storing the peer in the `Value` and using it to `control`.
```The content of context.Value is for maintainers not users. It should never be required input for documented or expected results.```
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment