Go concurrent TCP server hangs when JSON is sent as the response but works with plain string

Question

I am trying to implement a concurrent TCP server in Go and found this great explanatory article in linode where it clearly explains with a sample code. The sample code snippets for the client and server and included below.

Concurrent TCP server where for each TCP client a new go-routine is created.

package main

import (
        "bufio"
        "fmt"
        "net"
        "os"
        "strconv"
        "strings"
)

var count = 0

func handleConnection(c net.Conn) {
        fmt.Print(".")
        for {
                netData, err := bufio.NewReader(c).ReadString('\n')
                if err != nil {
                        fmt.Println(err)
                        return
                }

                temp := strings.TrimSpace(string(netData))
                if temp == "STOP" {
                        break
                }
                fmt.Println(temp)
                counter := strconv.Itoa(count) + "\n"
                c.Write([]byte(string(counter)))
        }
        c.Close()
}

func main() {
        arguments := os.Args
        if len(arguments) == 1 {
                fmt.Println("Please provide a port number!")
                return
        }

        PORT := ":" + arguments[1]
        l, err := net.Listen("tcp4", PORT)
        if err != nil {
                fmt.Println(err)
                return
        }
        defer l.Close()

        for {
                c, err := l.Accept()
                if err != nil {
                        fmt.Println(err)
                        return
                }
                go handleConnection(c)
                count++
        }
}

TCP client code snippet

package main

import (
        "bufio"
        "fmt"
        "net"
        "os"
        "strings"
)

func main() {
        arguments := os.Args
        if len(arguments) == 1 {
                fmt.Println("Please provide host:port.")
                return
        }

        CONNECT := arguments[1]
        c, err := net.Dial("tcp", CONNECT)
        if err != nil {
                fmt.Println(err)
                return
        }

        for {
                reader := bufio.NewReader(os.Stdin)
                fmt.Print(">> ")
                text, _ := reader.ReadString('\n')
                fmt.Fprintf(c, text+"\n")

                message, _ := bufio.NewReader(c).ReadString('\n')
                fmt.Print("->: " + message)
                if strings.TrimSpace(string(text)) == "STOP" {
                        fmt.Println("TCP client exiting...")
                        return
                }
        }
}

The above concurrent TCP server and client works without any issue. The issue comes when I change the TCP server to send a JSON response instead of the text response. When I change the line:

counter := strconv.Itoa(count) + "\n"
c.Write([]byte(string(counter)))

to

res, err := json.Marshal(IdentitySuccess{MessageType: "newidentity", Approved: "approved"})
if err != nil {
    fmt.Printf("Error: %v", err)
}
c.Write(res)

the server hangs and does not send any response back to client. The strange thing is that when I shut down the server forcefully with Ctrl+C, the server sends the response to the client. Any idea about this strange behavior? It's like the server holds the response and sends it when it exists.

Answer 1

take care to data races. You are writing and reading the counter variable from different routines without synchronization mechanisms. There is no benign data races.

Your implementation wont hit yet, because you are not testing simultaneous clients queries.

Enable the race detector by building your program using the -race flag, like this go run -race . / go build -race .

I have fixed the data race using the atomic package functions.

In below code, i have adjusted your code to use a bufio.Scanner instead of bufio.Reader, only for demonstration purposes.

    input := bufio.NewScanner(src)
    output := bufio.NewScanner(c)
    for input.Scan() {
        text := input.Text()
        fmt.Fprintf(c, "%v\n", text)
        isEOT := text == "STOP"

        if !output.Scan() {
            fmt.Fprintln(os.Stderr, output.Err())
            return
        }
        message := output.Text()
        fmt.Print("->: " + message)
        if isEOT {
            fmt.Println("All messages sent...")
            return
        }
    }

I also have adjusted the main sequence to simulate 2 consecutive clients, using a predefined buffer input that I reset along the way.

    input := `hello
world!
STOP
nopnop`
    test := strings.NewReader(input)

    go serve(arguments[1])
  
    test.Reset(input)
    query(arguments[1], test)

    test.Reset(input)
    query(arguments[1], test)

I added a simple retrier into your client, it helps us writing simple code.

    c, err := net.Dial("tcp", addr)
    for {
        if err != nil {
            fmt.Fprintln(os.Stderr, err)
            <-time.After(time.Second)
            c, err = net.Dial("tcp", addr)
            continue
        }
        break
    }

The overall program is assembled into one file, not really good to read the output, but easier to transport around and execute.

https://play.golang.org/p/keKQsKA3fAw

In below example I demonstrate how you can use a json marshaller / unmarshaller to exchange structured data.

    input := bufio.NewScanner(src)
    dst := json.NewEncoder(c)
    output := json.NewDecoder(c)
    for input.Scan() {
        text := input.Text()
        isEOT := text == "STOP"

        err = dst.Encode(text)
        if err != nil {
            fmt.Fprintln(os.Stderr, err)
            return
        }

        var tmp interface{}
        err = output.Decode(&tmp)
        if err != nil {
            fmt.Fprintln(os.Stderr, err)
            return
        }
        fmt.Printf("->: %v\n", tmp)
        if isEOT {
            fmt.Println("All messages sent...")
            return
        }
    }

But ! Beware, this last version is sensible to malicious users. Unlike bufio.Scanner or bufio.Reader it does not check the amount of data read on the wire. So it can possibly accumulate data until OOM.

This is particularly true for the server side of the thing, in

defer c.Close()
        defer atomic.AddUint64(&count, ^uint64(0))
        input := json.NewDecoder(c)
        output := json.NewEncoder(c)
        fmt.Print(".")
        for {
            var netData interface{}
            input.Decode(&netData)
            fmt.Printf("%v", netData)
            count := atomic.LoadUint64(&count)
            output.Encode(count)
            if x, ok := netData.(string); ok && x == "STOP" {
                break
            }
        }

https://play.golang.org/p/LpIu4ofpm9e

In your last piece of code, as answered by CodeCaster, don't forget to frame your messages using the appropriate delimiter.

Answer 2

That socket tutorial, just as so many other broken-by-design socket tutorials, doesn't explain at all what an application protocol is or why you need it. All it says is:

In this example, you have implemented an unofficial protocol that is based on TCP.

This "unofficial" protocol is as rudimentary as it gets: messages are separated by newline characters (\n).

You should not be using sockets like that in any environment, apart from learning the basics about sockets.

You need an application protocol to frame messages (so your client and server can recognise partial and concatenated messages).

So the short answer: send a \n after your JSON. The long answer: don't use barebones sockets like this, use an application protocol, such as HTTP.