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: return 0 instead of -1 when error occurred on a write #569

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

daynobug
Copy link
Contributor

@daynobug daynobug commented Apr 5, 2024

fix panic in case error not nil. m gets -1 when writes to a closed connection

@daynobug daynobug force-pushed the error-close-conn branch 2 times, most recently from c1298f7 to 6f153f8 Compare April 5, 2024 01:28
@panjf2000
Copy link
Owner

I appreciate your effort here, but I don't think returning -1 is allowed, see io.WriterTo and io.Writer. If a Writer doesn't comply with the interface contract and returns -1, I'd rather raise a panic for it. That's also what the Go std libraries do currently, check this out.

@daynobug
Copy link
Contributor Author

daynobug commented Apr 5, 2024

The -1 is actually returning by gnet itself.
I am writing a proxy by using gnet.Run and gnet.NewClient. Part of my code is kind of like that:

type ProxyContext struct {
	sync.Mutex
	frontend gnet.Conn
	backend  gnet.Conn
}

func (f *Frontend) OnTraffic(c gnet.Conn) gnet.Action {
	ctx := c.Context()
	proxyContext, o := ctx.(*ProxyContext)
	if !o || proxyContext == nil {
	}

	proxyContext.Lock()
	n, err := c.WriteTo(proxyContext.backend)
	if err != nil {
		log.Println(err)
		return gnet.Close
	}
	proxyContext.Unlock()
	log.Println("frontend write", n)

	return gnet.None
}

The frontend is using gnet.Conn::WriteTo to write data to the backend server, but the backend server has already closed. And then It just raised panic.
So I found gnet.Conn::Write is using gnet.Conn::write, and the gnet.Conn::write returns -1 and an error when the connection has closed.

return -1, os.NewSyscallError("write", err)

return c.write(p)

panic here because of the m is -1. panic: runtime error: slice bounds out of range [-1:]

c.buffer = c.buffer[m:]

At the same time, I found gnet.Conn::writev returns -1 too.

return -1, os.NewSyscallError("writev", err)

Maybe I should change both of the -1 to 0.

@panjf2000
Copy link
Owner

Returning -1 from a Write-like method is definitely a mistake and we should fix that kind of case.

connection_unix.go Outdated Show resolved Hide resolved
@panjf2000 panjf2000 changed the title bug: fix panic. m gets -1 when writes to a closed connection bug: return 0 instead of -1 when error occurred on a write Apr 5, 2024
@daynobug
Copy link
Contributor Author

daynobug commented Apr 5, 2024

done

@panjf2000 panjf2000 added bug Something isn't working pending merged This PR has been reviewed and approved labels Apr 5, 2024
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 79.08%. Comparing base (f5e5ef9) to head (cb1207e).

Files Patch % Lines
connection_unix.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #569   +/-   ##
=======================================
  Coverage   79.08%   79.08%           
=======================================
  Files          25       25           
  Lines        2109     2109           
=======================================
  Hits         1668     1668           
  Misses        305      305           
  Partials      136      136           
Flag Coverage Δ
unittests 79.08% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@panjf2000
Copy link
Owner

Thanks!

@panjf2000 panjf2000 merged commit b3ded75 into panjf2000:dev Apr 5, 2024
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pending merged This PR has been reviewed and approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants