ziutek / telnet

Package to handle a telnet connection
Other
142 stars 60 forks source link

Stuck on reading #15

Closed markx closed 6 years ago

markx commented 6 years ago

When I tried to use this client to connect to some mud servers, I found that sometimes it doesn't print all the text. After some digging, I found that the program got stuck on conn.Read when it's fed with a big buffer.

After more digging, I found out that the c.r.Buffered() returns more than 0, so it get stuck at c.ReadByte() in the next iteration. https://github.com/ziutek/telnet/blob/master/conn.go#L306

Also, I'm not sure if it's related, but usually it gets stuck in the middle on a line.

I'm not familiar with telnet or your client enough to solve this. Could you take a look?

Below is a snippet that can show the problem. You can run it multiple times to see how the problem is like.

func main() {
    conn, err := telnet.Dial("tcp", "bat.org:23")
    if err != nil {
        log.Fatal(err)
    }

    for {
        data := make([]byte, 512) // if I change the buffer size to 1, it doesn't hang
        n, err := conn.Read(data)
        if n > 0 {
            fmt.Print(string(data))
        }

        if err != nil {
            log.Fatal(err)
        }
    }
}
markx commented 6 years ago

I found the problem!

The Read call hangs when the servers send messages that end with a command, but ReadByte will try to read another byte.

I submitted a PR #16 , which has some API behavior changes... There could be other solutions, but let me know how you think about my changes.

ziutek commented 6 years ago

I've fixed this issue before read you PR.

It seems that tryReadByte works like intended.

The bug was in use ReadByte in Read instead of raw tryReadByte.

ziutek commented 6 years ago

After this fix, the Read can still hang after read real byte. I fixed this case. I added bat.org example. Fill free to improve it if you want. Try the last commit.

markx commented 6 years ago

Yeah thanks. This is a better fix.