-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
net/http: bufio.Writer aren't garbage collected after Hijack #59567
Comments
which bufio Reader / Writer are you referring to? |
codepackage main
import (
"github.com/lxzan/gws"
"log"
"net/http"
_ "net/http/pprof"
)
func main() {
handler := new(Handler)
upgrader := gws.NewUpgrader(handler, nil)
http.HandleFunc("/connect", func(writer http.ResponseWriter, request *http.Request) {
socket, err := upgrader.Accept(writer, request)
if err != nil {
log.Println(err.Error())
return
}
go socket.Listen()
})
if err := http.ListenAndServe(":8000", nil); err != nil {
log.Panic(err.Error())
}
}
type Handler struct {
gws.BuiltinEventHandler
}
func (c *Handler) OnPing(socket *gws.Conn, payload []byte) {
socket.WritePong(payload)
}
func (c *Handler) OnMessage(socket *gws.Conn, message *gws.Message) {
defer message.Close()
socket.WriteMessage(message.Opcode, message.Data.Bytes())
} Test Commandtcpkali -c 10000 --connect-rate 500 -r 1 -T 3000s -m 'hello' --ws 127.0.0.1:8000/connect pprof |
I believe this is working as intended. |
Never mind, I have implemented a websocket server based on tcp independently, and the memory footprint is significantly reduced. |
Hi @seankhliao, the returning of a write buffer you likely pointed at, Line 2118 in 35222ee
The other write buffer is attached to the connection, Lines 288 to 289 in 35222ee
As part of the hijack process, the connection read buffer, Line 325 in 35222ee
I do not see a need for allocating a new write buffer as part of the hijack process. After hijacking a connection, no further writes can be made via the The net/http unit tests are happy with the following patch and Patch: 25b0691 diff --git a/src/net/http/server.go b/src/net/http/server.go
index 9245778590..0ba55e8ee5 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -322,7 +322,7 @@ func (c *conn) hijackLocked() (rwc net.Conn, buf *bufio.ReadWriter, err error) {
rwc = c.rwc
rwc.SetDeadline(time.Time{})
- buf = bufio.NewReadWriter(c.bufr, bufio.NewWriter(rwc))
+ buf = bufio.NewReadWriter(c.bufr, c.bufw)
if c.r.hasByte {
if _, err := c.bufr.Peek(c.bufr.Buffered() + 1); err != nil {
return nil, nil, fmt.Errorf("unexpected Peek failure reading buffered byte: %v", err) BenchmarkServerHijack stats
I can open a PR with the patch in case you are happy with the approach, @seankhliao. |
Change https://go.dev/cl/634855 mentions this issue: |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
upgrade http to websocket
What did you expect to see?
After hijacking the http connection, the bufio.Writer in the connection is garbage collected
What did you see instead?
They have never been released
The text was updated successfully, but these errors were encountered: