Last active
October 31, 2018 23:45
-
-
Save dnldd/68a0a0d880a44bba7c3dfeb7b3f65255 to your computer and use it in GitHub Desktop.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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