First foray into #golang, writing a simple memcached clone, any feedback is appreciated

<p>I wanted to find a nice way to get started in Go and it seemed like writing a server socket app would be nice so I implemented a simple memcached server with only get/set/remove, source is available here: <a href="https://github.com/mauricio/gocached" rel="nofollow">https://github.com/mauricio/gocached</a></p> <p>Any feedback is appreciated :)</p> <hr/>**评论：**<br/><br/>TheMerovius: <pre><p>Hey. A couple of comments, in no particular order. The line numbers may be slightly off, as I haven&#39;t practiced good hygiene with my links. They should be relative to commit d5cf158c29e79d80de230e6a447b624917caea51, I <em>think</em>.</p> <ul> <li><a href="https://github.com/mauricio/gocached/blob/master/main.go#L13" rel="nofollow">this</a> looks relatively bad. <code>Start()</code> seems to be non-blocking, which means main() returns immediately and immediately stops the server. It is common and idiomatic in go, to just block forever (or until stopped), to make this simpler. If you need the server to be non-blocking, you can then just wrap it in a goroutine. Note that this will probably also somewhat simplify your implementation by a fair bit, because you&#39;ll be able to remove concurrency (and all the <code>isRunning</code> stuff).</li> <li>Consider to use typed constants <a href="https://github.com/mauricio/gocached/blob/master/server/tcp.go#L13" rel="nofollow">here</a>, e.g. do a <code>type errorCode byte</code> and then</li> </ul> <p>.</p> <pre><code>const ( errorValueTooLarge errorCode = 0x03 errorInvalidArguments = 0x04 ... ) </code></pre> <ul> <li>Why <a href="https://github.com/mauricio/gocached/blob/master/server/tcp.go#L41" rel="nofollow">int32 for a port</a>? Either make it an <code>int</code> (because who cares?) or an <code>uint16</code> (because that&#39;s the actual range of ports).</li> <li>Use <a href="http://godoc.org/log" rel="nofollow">log</a> instead of <code>fmt</code> for logging.</li> <li><a href="https://github.com/mauricio/gocached/blob/master/server/tcp.go#L125" rel="nofollow">this bit</a> seems wrong and racey to me (but see first point: It shouldn&#39;t even exist in the first place). The only thing <code>isRunning()</code> can guarantee is, that the <code>bool</code> is true <em>while that method is running</em>, which is a stupidly worthless guarantee. The server could have been stopped between <code>isRunning()</code> returning and the next line of the for-loop executing.</li> <li><code>defer</code> seems useless <a href="https://github.com/mauricio/gocached/blob/master/server/tcp.go#L137" rel="nofollow">here</a>, as the only way it&#39;s going to get called is when something <code>break</code>s out of the loop. Just put the call at the end. The only difference is, when something panic&#39;s in that loop, but in that case the process should die anyway and closing the connection shouldn&#39;t be necessary anyway.</li> <li>As an FYI (it&#39;s definitely premature optimization for a toy-project) <code>binary.Read</code> uses reflection, so is relatively slow. If you want to performance-optimize, you might consider replacing <a href="https://github.com/mauricio/gocached/blob/master/server/tcp.go#L141" rel="nofollow">this</a> by a series of <code>binary.BigEndian.Uint{16,32,64}</code> calls and the like.</li> <li>Typo: <code>defautEndiannes</code></li> <li>I&#39;m pretty sure, <a href="https://github.com/mauricio/gocached/blob/master/server/tcp.go#L152" rel="nofollow">those conversions</a> should be to <code>int</code>, no?</li> <li>I would also use <code>uint8</code> instead of <code>byte</code> in most places, but that&#39;s just personal taste</li> <li>It&#39;s unidiomatic to <a href="https://github.com/mauricio/gocached/blob/master/server/tcp.go#L191" rel="nofollow">use bool to signal errors</a>. Those methods should either return the error that occurred or be named as a condition (stupidly wordy, just to illustrate: &#34;handleGetSuccessful&#34;). But probably just return the error and check that for <code>nil</code>.</li> <li><a href="https://github.com/mauricio/gocached/blob/master/server/tcp.go#L207" rel="nofollow">typo</a>, <a href="https://github.com/mauricio/gocached/blob/master/server/tcp.go#L239" rel="nofollow">typo</a>, <a href="https://github.com/mauricio/gocached/blob/master/server/tcp.go#L253" rel="nofollow">typo</a></li> <li><a href="https://github.com/mauricio/gocached/blob/master/server/tcp.go#L224" rel="nofollow">this</a> looks like a DoS-attack to me by using unsanitized user-input (I could send a request with <code>TotalMessageBody == 0</code> and <code>KeyLength+ExtrasLength &gt; 0</code> and make you try to allocate negative amount of space which would panic and crash the server). You should <em>definitely</em> go through every request you get and make sure that all values given make sense, before doing <em>anything</em> with it. Treat every assumption about the protocol as false until proven true.</li> <li>I think <a href="https://github.com/mauricio/gocached/blob/master/server/tcp.go#L225" rel="nofollow">the duplication here</a> is unnecessary, as it&#39;s implied by only using one value.</li> <li><a href="https://github.com/mauricio/gocached/blob/master/server/tcp_test.go#L56" rel="nofollow">this message</a> makes it relatively hard to debug a failing test. I suggest putting both the expected and the actual value in the log-message (probably with the <code>%q</code> verb or something) so that you get all the useful information out of that testcase. Similarly for most log messages in that test.</li> <li>I feel like the methods in <a href="https://github.com/mauricio/gocached/blob/master/store/memory.go#L6" rel="nofollow">this interface</a> should also return an error to maintain flexibility in the interface - a later implementation might be able to fail and in that case you want to be able to signal that without panic&#39;ing</li> </ul> <p>That&#39;s all :)</p></pre>mauriciolinhares: <pre><p>wow, awesome!</p> <p>gonna work on that stuff!</p></pre>weberc2: <pre><p>I believe <a href="/u/bradfitz" rel="nofollow">/u/bradfitz</a> (the original author of memcached) also produced a Go analog. I don&#39;t recall the name, but it might be worth looking into.</p></pre>bradfitz: <pre><p>You thinking of <a href="https://github.com/golang/groupcache" rel="nofollow">https://github.com/golang/groupcache</a> ?</p></pre>weberc2: <pre><p>Yep. That was it.</p></pre>DevourerOfPizza: <pre><p>Yeah, it is great. I use it currently for my own projects and it is called <a href="https://github.com/bradfitz/gomemcache" rel="nofollow">gomemcache</a>.</p></pre>