Simple Go TCP server and client

Question

I'm a Go (and programming in general) newbie and the task was to program a Go server that would accept incoming messages, take each line, reverse its order and case and send it back. Besides doing that I also wrote a client (just for the hell of it and to help me test my server). I wonder if some things could've been done better and just want feedback on my code.

I'm particularly interested in proper usage of those break statements I used to forcefully finish Scan() loops and if I really have to send end of line symbols from my client as a hardcoded measure (rather than use some other function that automatically adds end of line symbols when reading input).

Server:

func reverse(s string) string {
 runes := []rune(s)
 for i, j := 0, len(runes)-1; i < j; i, j = i+1, j-1 {
 runes[i], runes[j] = runes[j], runes[i]
 }
 return string(runes)
}
func swapCase(s string) string {
 return strings.Map(func(r rune) rune {
 switch {
 case unicode.IsLower(r):
 return unicode.ToUpper(r)
 case unicode.IsUpper(r):
 return unicode.ToLower(r)
 }
 return r
 }, s)
}
func handleServerConnection(c net.Conn, i int) {
 for {
 // scan message
 scanner := bufio.NewScanner(c)
 for scanner.Scan() {
 msg := scanner.Text()
 log.Printf("Client %v sends: %v", i, msg)
 msgNew := swapCase(reverse((msg)))
 c.Write([]byte(msgNew + "\n"))
 log.Printf("Client %v receives: %v", i, msgNew)
 fmt.Println("---")
 }
 if errRead := scanner.Err(); errRead != nil {
 log.Printf("Client %v disconnected...", i)
 fmt.Println("---")
 return
 }
 }
}
func main() {
 log.Println("Server launched...")
 // listen on all interfaces
 ln, _ := net.Listen("tcp", ":9999")
 i := 0
 for {
 // accept connection on port
 c, _ := ln.Accept()
 i++
 log.Printf("Client %v connected...", i)
 fmt.Println("---")
 // handle the connection
 go handleServerConnection(c, i)
 }
}

Client:

func main() {
 i := 0 // connection index
 // connect to server
 for {
 connect:
 c, errConn := net.Dial("tcp", "127.0.0.1:9999")
 if errConn != nil {
 continue
 } else {
 i++
 if i <= 1 {
 log.Println("Connected to server...")
 fmt.Println("---")
 } else {
 log.Println("Reconnected to server...")
 fmt.Println("---")
 }
 }
 for {
 // read in input from stdin
 scannerStdin := bufio.NewScanner(os.Stdin)
 fmt.Print("Server message: ")
 for scannerStdin.Scan() {
 text := scannerStdin.Text()
 fmt.Println("---")
 // send to server
 _, errWrite := fmt.Fprintf(c, text+"\n")
 if errWrite != nil {
 log.Println("Server offline, attempting to reconnect...")
 goto connect
 }
 log.Print("Server receives: " + text)
 break
 }
 // listen for reply
 scannerConn := bufio.NewScanner(c)
 for scannerConn.Scan() {
 log.Println("Server sends: " + scannerConn.Text())
 break
 }
 if errReadConn := scannerStdin.Err(); errReadConn != nil {
 log.Printf("Read error: %T %+v", errReadConn, errReadConn)
 return
 }
 fmt.Println("---")
 }
 }
}

Answer 1

Your reverse, and swapCase functions are good. The native use of runes is great, and the functions are clear, etc. If I was to really nit-pick, I would suggest a real function (instead of an anonymous) for the switch statement.

func swapRuneCase(r rune) rune {
 switch {
 case unicode.IsLower(r):
 return unicode.ToUpper(r)
 case unicode.IsUpper(r):
 return unicode.ToLower(r)
 }
 return r
}

That makes it clearer, and potentially, later, reusable.

It also makes the swapCase function look like:

func swapCase(s string) string {
 return strings.Map(swapRuneCase, s)
}

Interestingly, it also leads to a more rune-based server handler function...

Here's your function:

func handleServerConnection(c net.Conn, i int) {
 for {
 // scan message
 scanner := bufio.NewScanner(c)
 for scanner.Scan() {
 msg := scanner.Text()
 log.Printf("Client %v sends: %v", i, msg)
 msgNew := swapCase(reverse((msg)))
 c.Write([]byte(msgNew + "\n"))
 log.Printf("Client %v receives: %v", i, msgNew)
 fmt.Println("---")
 }
 if errRead := scanner.Err(); errRead != nil {
 log.Printf("Client %v disconnected...", i)
 fmt.Println("---")
 return
 }
 }
}

That function is doing too many things .... hmmm actually it's just got a useless outer loop. The "scan" loop is all you need. The scanner.Scan() will only terminate the loop when the connection is closed (the c Reader returns an error - presumably EOF, but any other error has the same consequence... c is not readable). So, remove the outer loop, and, we know that the scanner will return an error too, so the condition at the end is not useful. This leaves:

func handleServerConnection(c net.Conn, i int) {
 // scan message
 scanner := bufio.NewScanner(c)
 for scanner.Scan() {
 msg := scanner.Text()
 log.Printf("Client %v sends: %v", i, msg)
 msgNew := swapCase(reverse((msg)))
 c.Write([]byte(msgNew + "\n"))
 log.Printf("Client %v receives: %v", i, msgNew)
 fmt.Println("---")
 }
 log.Printf("Client %v disconnected...", i)
 fmt.Println("---")
}

Note that the fiddling you do with the newline is necessary because you can't "reverse" it too.

Your mixing of both log and fmt in things like log.Printf and fmt.Println is confusing. Use one of them, not both. log is probably the better option. Also, you should be adding \n to the fmt.Printf format strings (you don't need to do that for log.Printf - but you can if you want). There's too much logging! You're not inspecting the error value for Write(...), and you're not closing the Conn which will lead to "CLOSE_WAIT" sockets in the OS network stack. In general, though. I would rationalize it all down to:

func handleServerConnection(c net.Conn, i int) {
 defer func() {
 if err := c.Close(); err != nil {
 log.Printf("Closing client %v connection returned error: %v\n", i, err)
 }
 }()
 // scan message
 scanner := bufio.NewScanner(c)
 for scanner.Scan() {
 msg := scanner.Text()
 msgNew := swapCase(reverse((msg)))
 if _, err := c.Write([]byte(msgNew + "\n")); err != nil {
 log.Printf("Client %v has closed writer: %v\n", i, err)
 break
 }
 log.Printf("Client %v\n received: %v\n returned: %v\n", i, msg, msgNew)
 }
 log.Printf("Client %v disconnected...\n", i)
}

Right, that's it for the handler function.

Your main function is structurally OK, but you MUST check the error state from the Listen call:

ln, _ := net.Listen("tcp", ":9999")

By ignoring the error, if you start your program twice, the second program will panic with a nil pointer reference on the ln.Accept() call (since ln will be nil)

That Accept() call should also have it's error checked too!

Again, using both log and fmt is confusing.

In general, you should specialize your network services/processes when you can. You use net.Listen and ln.Accept when you should probably use ListenTCP and AcceptTCP (you get a TCPListener instead of a Listener) - and your connection is a TCPConn and not a Conn (you should change the parameter type on the handler function too).

Your client side has similar issues, but I think you can figure them out as you go based on the above.

• Arthmost: Thank you for your thorough code review! I've derived a lot of useful things from it, but also let me explain why I've done a few things this way - I'm using fmt and log interchangeably because sometimes I want the date and time printed in my terminal and sometimes I just want the ---. That's pretty much it. I've pretty much fixed all the server code issues and rewrote my client. The only confusing bug that remains is in the 'listen for response' part of my client - it just hangs there because it doesn't receive an EOF from the server. I don't know how to proceed there.