panjf2000 / gnet

🚀 gnet is a high-performance, lightweight, non-blocking, event-driven networking framework written in pure Go.
https://gnet.host
Apache License 2.0
9.69k stars 1.04k forks source link

bug: return 0 instead of -1 when error occurred on a write #569

Closed daynobug closed 7 months ago

daynobug commented 7 months ago

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

panjf2000 commented 7 months ago

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 commented 7 months ago

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.

https://github.com/panjf2000/gnet/blob/f5e5ef9856dfa0b68b1702048508c1fcd14aae43/connection_unix.go#L152

https://github.com/panjf2000/gnet/blob/f5e5ef9856dfa0b68b1702048508c1fcd14aae43/connection_unix.go#L364

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

https://github.com/panjf2000/gnet/blob/f5e5ef9856dfa0b68b1702048508c1fcd14aae43/connection_unix.go#L387

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

https://github.com/panjf2000/gnet/blob/f5e5ef9856dfa0b68b1702048508c1fcd14aae43/connection_unix.go#L186

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

panjf2000 commented 7 months ago

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

daynobug commented 7 months ago

done

codecov[bot] commented 7 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/panjf2000/gnet/pull/569/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andy+Pan) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/panjf2000/gnet/pull/569/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andy+Pan) | `79.08% <0.00%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andy+Pan#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

panjf2000 commented 7 months ago

Thanks!