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

WebRTC: TCP transport should use read_fully instead of read. v5.0.194 v6.0.94 #3847

Merged

Conversation

chundonglinlin
Copy link
Member

@chundonglinlin chundonglinlin commented Oct 20, 2023

SRS supports TCP WebRTC by reading 2 bytes of length, like read(buf, 2). However, in some cases, it might receive 1 byte, causing subsequent data to be incorrect and making it unable to push or play streams.


Co-authored-by: john hondaxiao@tencent.com

@sandro-qiang
Copy link

you do it wrong, use read_fully instead of read, you use read instead of read_fully

@chundonglinlin chundonglinlin force-pushed the bugfix/use-read-packet-for-rtc-tcp branch from 7349441 to f24ea28 Compare October 20, 2023 06:49
@chundonglinlin
Copy link
Member Author

chundonglinlin commented Oct 20, 2023

ou do it wrong, use read_fully instead of read, you use read instead of read_ful

Sorry, I misunderstood earlier and have made some changes. Could you please create a simple demo to test it? @sandro-qiang

TRANS_BY_GPT4

@chundonglinlin chundonglinlin changed the title SrsRtcTcpConn: use read instead of read_fully. SrsRtcTcpConn: use read_fully instead of read. Oct 20, 2023
@chundonglinlin
Copy link
Member Author

chundonglinlin commented Oct 20, 2023

我用gpt写了个golang demo,使用TCP NODELAY选项发送单个字节的情况
测试命令:go run test.go

package main

import (
	"fmt"
	"net"
	"time"
)

func main() {
	// Set server address and port
	serverAddr := "localhost:8000"

	// connect server
	conn, err := net.Dial("tcp", serverAddr)
	if err != nil {
		fmt.Println("cannot connect server:", err)
		return
	}
	defer conn.Close()

	// Send a single byte of data
	data := []byte{0x01}
	_, err = conn.Write(data)
	if err != nil {
		fmt.Println("data send fail:", err)
		return
	}

	fmt.Println("data send success")

	time.Sleep(time.Second)

	fmt.Println("test finish.")
}

SRS调用SrsRtcTcpConn::read_packet

分支 接口 结果
bugfix/use-read-packet-for-rtc-tcp skt_->read_fully 等待后断开连接
develop skt_->read 立即读取1个字节输出

@sandro-qiang
Copy link

sandro-qiang commented Oct 20, 2023

I have written a Golang demo using GPT, which sends a single byte using the TCP NODELAY option. Test command: go run test.go

package main

import (
	"fmt"
	"net"
	"time"
)

func main() {
	// Set server address and port
	serverAddr := "localhost:8000"

	// connect server
	conn, err := net.Dial("tcp", serverAddr)
	if err != nil {
		fmt.Println("cannot connect server:", err)
		return
	}
	defer conn.Close()

	// Send a single byte of data
	data := []byte{0x01}
	_, err = conn.Write(data)
	if err != nil {
		fmt.Println("data send fail:", err)
		return
	}

	fmt.Println("data send success")

	time.Sleep(time.Second)

	fmt.Println("test finish.")
}

After SRS calls SrsRtcTcpConn::read_packet

Branch Interface Result
bugfix/use-read-packet-for-rtc-tcp skt_->read_fully Waits then disconnects
develop skt_->read Immediately reads 1 byte output

This test cannot reproduce the problem, because whether you turn on no-delay or not, you only sent one byte, so the read packet length will always fail.
The real problem is that at some random time, an error is reported and the connection is disconnected. The error could be that the packet length exceeds 1500, the ssrc does not exist, the packet is unknown, etc.
This error is not easy to reproduce 100% through unit testing, because without no_delay, the TCP stack will decide how to divide the IP packet.

TRANS_BY_GPT4

@sandro-qiang
Copy link

sandro-qiang commented Oct 20, 2023

I have written a Golang demo using GPT, which sends a single byte using the TCP NODELAY option. Test command: go run test.go

package main

import (
	"fmt"
	"net"
	"time"
)

func main() {
	// Set server address and port
	serverAddr := "localhost:8000"

	// connect server
	conn, err := net.Dial("tcp", serverAddr)
	if err != nil {
		fmt.Println("cannot connect server:", err)
		return
	}
	defer conn.Close()

	// Send a single byte of data
	data := []byte{0x01}
	_, err = conn.Write(data)
	if err != nil {
		fmt.Println("data send fail:", err)
		return
	}

	fmt.Println("data send success")

	time.Sleep(time.Second)

	fmt.Println("test finish.")
}

After SRS calls SrsRtcTcpConn::read_packet

Branch
Interface
Result

bugfix/use-read-packet-for-rtc-tcp
skt_->read_fully
Waits then disconnects

develop
skt_->read
Immediately reads 1 byte output

This test cannot reproduce the problem, because whether you turn on no-delay or not, you only sent one byte, so the read packet length will always fail. The real problem is that at some random time, an error is reported and the connection is disconnected. The error could be that the packet length exceeds 1500, the ssrc does not exist, the packet is unknown, etc. This error is not easy to reproduce 100% through unit testing, because without no_delay, the TCP stack will decide how to divide the IP packet.

TRANS_BY_GPT4

Perhaps you can try this: send one byte, then wait for 5 seconds, and send another byte. The srs server will print the packet length. The printed lengths in these two situations will definitely be different. Moreover, the printed length from the 'read' operation is incorrect because it only read one byte.

@winlinvip winlinvip changed the title SrsRtcTcpConn: use read_fully instead of read. WebRTC: TCP transport SrsRtcTcpConn should use read_fully instead of read. Oct 21, 2023
@winlinvip winlinvip changed the title WebRTC: TCP transport SrsRtcTcpConn should use read_fully instead of read. WebRTC: TCP transport should use read_fully instead of read. Oct 21, 2023
@winlinvip
Copy link
Member

winlinvip commented Oct 21, 2023

Need to merge into 5 and 6 @xiaozhihong

TRANS_BY_GPT4

@xiaozhihong xiaozhihong changed the title WebRTC: TCP transport should use read_fully instead of read. WebRTC: TCP transport should use read_fully instead of read. v5.0.194 v6.0.94 Oct 21, 2023
@xiaozhihong xiaozhihong added the RefinedByAI Refined by AI/GPT. label Oct 21, 2023
@winlinvip winlinvip merged commit 9b07d84 into ossrs:develop Oct 23, 2023
17 checks passed
winlinvip pushed a commit that referenced this pull request Oct 23, 2023
…#3847)

SRS supports TCP WebRTC by reading 2 bytes of length, like `read(buf,
2)`. However, in some cases, it might receive 1 byte, causing subsequent
data to be incorrect and making it unable to push or play streams.

---------

Co-authored-by: john <hondaxiao@tencent.com>
duiniuluantanqin pushed a commit to duiniuluantanqin/srs that referenced this pull request Oct 27, 2023
…ossrs#3847)

SRS supports TCP WebRTC by reading 2 bytes of length, like `read(buf,
2)`. However, in some cases, it might receive 1 byte, causing subsequent
data to be incorrect and making it unable to push or play streams.

---------

Co-authored-by: john <hondaxiao@tencent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RefinedByAI Refined by AI/GPT.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SrsRtcTcpConn::read_packet should use read_fully instead of read
4 participants