Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Read returns io.ErrShortBuffer on closed connection #598

Closed
3 tasks done
ezh opened this issue Apr 28, 2024 · 9 comments
Closed
3 tasks done

[Bug]: Read returns io.ErrShortBuffer on closed connection #598

ezh opened this issue Apr 28, 2024 · 9 comments
Assignees
Labels
bug Something isn't working needs more info waiting for response waiting for the response from commenter

Comments

@ezh
Copy link

ezh commented Apr 28, 2024

Actions I've taken before I'm here

  • I've thoroughly read the documentations on this issue but still have no clue.
  • I've searched the Github Issues but didn't find any duplicate issues that have been resolved.
  • I've searched the internet for this issue but didn't find anything helpful.

What happened?

https://github.com/panjf2000/gnet/blob/master/connection_unix.go#L306

func (c *conn) Read(p []byte) (n int, err error) {
	if c.inboundBuffer.IsEmpty() {
		n = copy(p, c.buffer)
		c.buffer = c.buffer[n:]
->		if n == 0 && len(p) > 0 {
			err = io.ErrShortBuffer
		}
		return
	}
	n, _ = c.inboundBuffer.Read(p)
	if n == len(p) {
		return
	}
	m := copy(p[n:], c.buffer)
	n += m
	c.buffer = c.buffer[m:]
	return
}

A closed connection with an empty buffer(because of a lack of incoming data) returns a ShortBuffer error

image

NB. opened connection also returns a shortbuffer when there is no incoming data, but it is not so critical 🙈 .

Major version of gnet

v2

Specific version of gnet

v2.5.1

Operating system

macOS

OS version

Darwin 23.4.0 arm64

Go version

go version go1.22.2 darwin/arm64

Relevant log output

The observability is far from the best since it is the simplest case.

[gnet] 2024-04-28T15:52:50.015965+04:00 INFO    logging/logger.go:256   Starting gnet client with 1 event-loop
[gnet] 2024-04-28T15:52:50.016327+04:00 DEBUG   logging/logger.go:249   default logging level is DEBUG
[gnet] 2024-04-28T15:52:56.135536+04:00 DEBUG   v2/reactor_default.go:105       event-loop(0) is exiting in terms of the demand from user, gnet: server is going to be shutdown

Code snippets (optional)

No response

How to Reproduce

create listener
like

	conn, err := l.Listener.Accept()
	if err != nil {
		return nil, err
	}
	gConn, err := client.EnrollContext(conn, l)
	if err != nil {
		return nil, l.GetError()
	}

And connect with the TCP session.

clientConn, err := net.Dial("tcp", l.Addr().String())
Expect(err).NotTo(HaveOccurred())

Than, on an empty connection, do

n, err := conn.Read(buf[totalRead:])

it will return io.ErrShortBuffer

close connection in a go routine

do conn.Read on more time on closed connection and 🤷‍♂️

Does this issue reproduce with the latest release?

It can reproduce with the latest release

@ezh ezh added the bug Something isn't working label Apr 28, 2024
@gh-translator gh-translator changed the title [Bug]: Read returns [Bug]: Read returns Apr 28, 2024
@ezh ezh changed the title [Bug]: Read returns [Bug]: Read returns io.ErrShortBuffer on close connection Apr 28, 2024
@ezh ezh changed the title [Bug]: Read returns io.ErrShortBuffer on close connection [Bug]: Read returns io.ErrShortBuffer on closed connection Apr 28, 2024
@ezh
Copy link
Author

ezh commented Apr 28, 2024

I can apply a workaround, but certainly, preemptively checking the connection status before each read would not be the most efficient or elegant solution.

@ezh
Copy link
Author

ezh commented Apr 28, 2024

Honestly, I'm quite surprised that huge corporations are using the code with such behavior in production. Thank you for the code, anyway.

@panjf2000
Copy link
Owner

I fail to see what is bothering you. What's your problem with io.ErrShortBuffer?

@panjf2000 panjf2000 added waiting for response waiting for the response from commenter needs more info labels Apr 28, 2024
@ezh
Copy link
Author

ezh commented Apr 28, 2024

io.ErrShortBuffer indicates at the same time:

  1. there is a short buffer
  2. there is not enough data
  3. there is a closed connection
    For example, if there is not enough data, the read operation should be repeated after a while to get a new portion. However, there is no reason to do that in case of a closed connection.
    Am I wrong?
    I may have missed an example of how to do it correctly.

We expect to get 3 bytes.

  1. conn.Read returns io.ErrShortBuffer because there is no data at all
  2. do conn.Read one more time, the client sends 1 byte, and we have io.ErrShortBuffer again
  3. the connection took too long, we close the connection in a separate goroutine
  4. conn.Read returns io.ErrShortBuffer again

Do you think the behavior is correct? If so, I'll close the report. 🤷‍♂️

@ezh
Copy link
Author

ezh commented Apr 28, 2024

I understand that we can track the status with the event handler, or even write the custom state machine.

But my question is about conn.Read behavior.

@panjf2000
Copy link
Owner

gnet is a event-driven framework, you should only read data in OnTraffic, why would you call Read() constantly to check if there is data coming? I don't think you're using gnet in the right way.

@ezh
Copy link
Author

ezh commented Apr 28, 2024

Thank you for your quick feedback. I appreciate your guidance and will verify the usage to align with best practices.

Maybe I didn't get the idea.

@ezh ezh closed this as completed Apr 28, 2024
@happyhakka6
Copy link

var ErrIncompletePacket = errors.New("incomplete packet")

const (
PacketSize = 4 //tcp包报文协议格式头4个字节,用于标识头+包体大小
)

func (codec TcpCodec) Decode(c gnet.Conn) (*quote.BaseMsg, int, error) {
buf, err := c.Next(PacketSize)
if err != nil {
if errors.Is(err, io.ErrShortBuffer) {
err = ErrIncompletePacket
}
return nil, 0, err
}

bodyLen := binary.LittleEndian.Uint32(buf[0:PacketSize])
buf1, err := c.Peek(int(bodyLen))
defer c.Discard(int(bodyLen))
if err != nil {
	if errors.Is(err, io.ErrShortBuffer) {
		err = ErrIncompletePacket
	}
	return nil, 0, err
}

msg := &quote.BaseMsg{}
err = proto.Unmarshal(buf1, msg)
if err != nil {
	return nil, 0, err
}
return msg, int(bodyLen), nil

}

我在 OnTraffic 中调用 Decode 也报同样的错误, io.ErrShortBuffer 导致无法正常解析后续 tcp 报文,
如何规避这个错误?

@gh-translator
Copy link
Collaborator

🤖 Non-English text detected, translating ...


var ErrIncompletePacket = errors.New("incomplete packet")

const (
PacketSize = 4 //The first 4 bytes of the tcp packet message protocol format are used to identify the header + packet body size
)

func (codec TcpCodec) Decode(c gnet.Conn) (*quote.BaseMsg, int, error) {
buf, err := c.Next(PacketSize)
if err != nil {
if errors.Is(err, io.ErrShortBuffer) {
err = ErrIncompletePacket
}
return nil, 0, err
}

bodyLen := binary.LittleEndian.Uint32(buf[0:PacketSize])
buf1, err := c.Peek(int(bodyLen))
defer c.Discard(int(bodyLen))
if err != nil {
if errors.Is(err, io.ErrShortBuffer) {
err = ErrIncompletePacket
}
return nil, 0, err
}

msg := &quote.BaseMsg{}
err = proto.Unmarshal(buf1, msg)
if err != nil {
return nil, 0, err
}
return msg, int(bodyLen), nil
}

I also reported the same error when calling Decode in OnTraffic. io.ErrShortBuffer caused the subsequent tcp messages to be unable to be parsed normally.
How to avoid this error?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs more info waiting for response waiting for the response from commenter
Projects
None yet
Development

No branches or pull requests

4 participants