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

splithttp Read() using blocking mode #3473

Merged
merged 4 commits into from
Jun 24, 2024
Merged

Conversation

maskedeken
Copy link
Contributor

splithttp的Read()应该使用阻塞模式,不然trojan和VLESS读取第一个数据包时会返回长度为0

Copy link
Collaborator

@mmmray mmmray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the reproduction steps. This hangs correctly:

func Test_readzero(t *testing.T) {
    q := NewUploadQueue(10)
    buf := make([]byte, 20)
    n, err := q.Read(buf)
    common.Must(err)
    if n == 0 {
        t.Error("n=0")
    }
}

if len(h.heap) == 0 {
packet, more := <-h.pushedPackets
if !more {
return 0, io.EOF
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if EOF is handled here, I think line 50 can be removed? and then h.closed can be removed.

because h.pushedPackets is always closed when h.closed == true

@maskedeken
Copy link
Contributor Author

I don't understand the reproduction steps. This hangs correctly:

func Test_readzero(t *testing.T) {
    q := NewUploadQueue(10)
    buf := make([]byte, 20)
    n, err := q.Read(buf)
    common.Must(err)
    if n == 0 {
        t.Error("n=0")
    }
}

Splithttp doesn't work with Trojan protocol, u can give it a try

@maskedeken
Copy link
Contributor Author

I don't understand the reproduction steps. This hangs correctly:

func Test_readzero(t *testing.T) {
    q := NewUploadQueue(10)
    buf := make([]byte, 20)
    n, err := q.Read(buf)
    common.Must(err)
    if n == 0 {
        t.Error("n=0")
    }
}

it hangs here

packet, more := <-h.pushedPackets

but it returns 0 directly when h.pushedPackets gets data

@mmmray
Copy link
Collaborator

mmmray commented Jun 24, 2024

thanks, i realized it after you said trojan is broken, and pushed a unittest. I could not reproduce it with VLESS, only trojan.

it seems httpupgrade might have a similar issue here (but for downloaded data)

i think in addition to this PR, trojan should also handle 0-length reads better, otherwise the issue has to be considered for N transports maybe not necessary

Copy link
Collaborator

@mmmray mmmray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good but i don't dare to merge

@yuhan6665 yuhan6665 changed the title splithttp的Read()应该使用阻塞模式 splithttp Read() using blocking mode Jun 24, 2024
@yuhan6665 yuhan6665 merged commit e4f9d03 into XTLS:main Jun 24, 2024
34 checks passed
@yuhan6665
Copy link
Member

Thanks guys for the great collaboration! @mmmray now that you can maintain xray-core,, you are welcomed to merge, at least with code you are familiar with :)

@yuhan6665
Copy link
Member

Some points are:

  • use squash if one change has multiple commits
  • commit messages must be in English (In case of this pr, because the original title was in Chinese, it need to be re-write before squash)
  • if you run into any issues, feel free to ping me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants