XHTTP: pooled HTTP/1.1 upload connections are only closed by the GC

[StateException]

Integrity requirements

• I have read all the comments in the issue template and ensured that this issue meet the requirements.
• I confirm that I have read the documentation, understand the meaning of all the configuration items I wrote, and did not pile up seemingly useful options or default values.
• I provided the complete config and logs, rather than just providing the truncated parts based on my own judgment.
• I searched issues and did not find any similar issues.
• The problem can be successfully reproduced in the latest Release

Description

I was looking into why an xhttp client keeps more open sockets than I expected, and ended up in the HTTP/1.1 upload path.

PostPacket() puts the upload connections into a sync.Pool and takes them back out on the next packet:

Xray-core/transport/internet/splithttp/client.go

Lines 127 to 167 in d2758a0

	uploadConn = c.uploadRawPool.Get()
	newConnection := uploadConn == nil
	if newConnection {
	newConn, err := c.dialUploadConn(context.WithoutCancel(ctx))
	if err != nil {
	return err
	}
	h1UploadConn = NewH1Conn(newConn)
	uploadConn = h1UploadConn
	} else {
	h1UploadConn = uploadConn.(*H1Conn)
	// TODO: Replace 0 here with a config value later
	// Or add some other condition for optimization purposes
	if h1UploadConn.UnreadedResponsesCount > 0 {
	resp, err := http.ReadResponse(h1UploadConn.RespBufReader, req)
	if err != nil {
	c.closed = true
	return fmt.Errorf("error while reading response: %s", err.Error())
	}
	io.Copy(io.Discard, resp.Body)
	defer resp.Body.Close()
	if resp.StatusCode != 200 {
	return fmt.Errorf("got non-200 error response code: %d", resp.StatusCode)
	}
	}
	}
	_, err := h1UploadConn.Write(requestBuff.Bytes())
	// if the write failed, we try another connection from
	// the pool, until the write on a new connection fails.
	// failed writes to a pooled connection are normal when
	// the connection has been closed in the meantime.
	if err == nil {
	break
	} else if newConnection {
	return err
	}
	}
	c.uploadRawPool.Put(uploadConn)

As far as I can tell nothing ever closes them. There is no idle timeout, no size limit, and no owner that could close them, so a pooled connection stays open until the collector drops the pool contents and the finalizer of the underlying net.Conn runs.

That looks out of place next to everything else the same file sets up, where each connection cache does have an idle bound:

• http2.Transport — IdleConnTimeout: net.ConnIdleTimeout (dialer.go#L313)
• http.Transport — IdleConnTimeout: net.ConnIdleTimeout (dialer.go#L324)
• quic — quicConfig.MaxIdleTimeout = net.ConnIdleTimeout (dialer.go#L178)

and the constant itself reads like it is meant to cover everything:

// defines the maximum time an idle TCP session can survive in the tunnel, so
// it should be consistent across HTTP versions and with other transports.
const ConnIdleTimeout = 300 * time.Second

To make sure it is really the collector doing the closing, and not the peer or the kernel, I ran the same transfer twice on loopback with v26.3.27, xhttp + packet-up + alpn: ["http/1.1"]: 60 MB through the tunnel, then sampling the client's outbound ESTABLISHED sockets to the server port every 5 s.

GOGC default   peak 39 sockets, 0 after 270 s, in steps: 39 -> 31 -> 16 -> 0
GOGC=off       peak 54 sockets, still 46 after 400 s, never drops

With the collector off they simply stay. The steps in the first run line up with GC cycles, which also fits sync.Pool: its victim cache survives one cycle and dies on the second.

The server does not close them either. The XHTTP listener only sets ReadHeaderTimeout (hub.go#L584), and with ReadTimeout unset http.Server.IdleTimeout ends up as zero, so from the server side these are just idle keep-alive connections that nobody is in a hurry to drop.

I then tried it with xmux, expecting that retiring a client would take its pool along, but it does not: GetXmuxClient() removes the client from the slice without closing anything. In a run with hMaxRequestTimes: 10 there were 6 retirements and the sockets still went to zero only at 270 s, together with the same GC cycles.

One smaller thing I noticed while measuring: sync.Pool is sharded per P, so Get() returns nil when the connection happens to sit in another shard, and a new one is dialled instead. Over a 40 MB transfer with 4 concurrent sessions Tcp.ActiveOpens from /proc/net/snmp grew by 49-54, which is a lot more dials than a pool of this size should need.

Is the collector meant to be the only owner of these connections here?

Reproduction Method

Nothing special is needed, any sustained upload over packet-up with alpn: ["http/1.1"] will do. What I did:

• pushed 60 MB through the SOCKS inbound, in a few concurrent sessions, into a local TCP server that reads and discards
• watched the client's outbound sockets with ss -tnp | grep 10900 every 5 s until they went away
• repeated the same run with GOGC=off in the client's environment

It does not reproduce with REALITY, where decideHTTPVersion() always returns "2", and it does not reproduce with stream-one/stream-up, which do not go through PostPacket().

Client config

Details

{
  "log": { "loglevel": "debug" },
  "inbounds": [
    {
      "listen": "127.0.0.1",
      "port": 10808,
      "protocol": "socks",
      "settings": { "udp": true }
    }
  ],
  "outbounds": [
    {
      "protocol": "vless",
      "settings": {
        "vnext": [
          {
            "address": "127.0.0.1",
            "port": 10900,
            "users": [
              {
                "id": "37ad3051-1de6-486e-aef0-4f88cb16143d",
                "encryption": "none"
              }
            ]
          }
        ]
      },
      "streamSettings": {
        "network": "xhttp",
        "xhttpSettings": {
          "path": "/x",
          "mode": "packet-up"
        },
        "security": "tls",
        "tlsSettings": {
          "serverName": "localhost",
          "alpn": ["http/1.1"],
          "allowInsecure": true
        }
      }
    }
  ]
}

Server config

Details

{
  "log": { "loglevel": "debug" },
  "inbounds": [
    {
      "listen": "127.0.0.1",
      "port": 10900,
      "protocol": "vless",
      "settings": {
        "clients": [{ "id": "37ad3051-1de6-486e-aef0-4f88cb16143d" }],
        "decryption": "none"
      },
      "streamSettings": {
        "network": "xhttp",
        "xhttpSettings": {
          "path": "/x",
          "mode": "packet-up"
        },
        "security": "tls",
        "tlsSettings": {
          "alpn": ["http/1.1"],
          "certificates": [
            {
              "certificateFile": "/path/to/cert.pem",
              "keyFile": "/path/to/key.pem"
            }
          ]
        }
      }
    }
  ],
  "outbounds": [{ "protocol": "freedom" }]
}

Client log

n/a - with loglevel: debug there is nothing about this in the log, the connections are simply never closed

Server log

n/a

[Fangliding]

似乎是没有关闭策略 我会研究一下chromium的回收策略

[embogomolov]

The idle bounds you listed for that file line up with something I hit one step earlier, on the dial rather than the pool.

dialContext in dialer.go receives ctxInner from DialTLSContext, and on the h2 path that context comes from the context.WithoutCancel in OpenStream, the function right above the pool code you linked. net.Dialer.Timeout ends the TCP phase at 16s, but reality.UClient and tls.UConn.HandshakeContext after it have no bound at all, and clientConnPool.getClientConn waits on <-call.done with no select on the context. A peer that finishes the TCP handshake and then goes quiet keeps that goroutine for the life of the process.

I filed it as #6590, with a listener that accepts TCP and never writes, so it reproduces in a couple of minutes on the release binary.