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.

Sign up for GitHub

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

Jump to bottom

修复mux子连接无法正常断开的问题 #1417

Merged
merged 1 commit into from
Dec 10, 2022
Merged

修复mux子连接无法正常断开的问题 #1417

merged 1 commit into from
Dec 10, 2022

Conversation

renahita6
Copy link

ray系的mux一直有一个老毛病，取消下载文件后流量还会继续跑，比如#232。
排查后发现问题出在commom/mux/server.go文件下的handleStatusEnd函数。
具体而言，mux客户端向服务端发送End状态请求结束对应的mux.Session时，服务端接受到该请求，并最终进入handleStatusEnd函数：

func (w *ServerWorker) handleStatusEnd(meta *FrameMetadata, reader *buf.BufferedReader) error {
	if s, found := w.sessionManager.Get(meta.SessionID); found {
		...
		s.Close()
	}
	...
	return nil
}

上述if语句中的s.Close函数：

func (s *Session) Close() error {
	common.Close(s.output)
	common.Close(s.input)
	s.parent.Remove(s.ID)
	return nil
}

分别调用s.input和s.output的Close方法，然后将该session的ID从sessionmanager中移除。
问题出现在，s.input是一个pipe.Reader，在xray中pipe.Reader没有实现Close方法，导致子连接无法正常断开，远程服务器会持续通过该pipe.Reader传输数据。
解决方法很简单，在s.Close()前手动中止pipe.Reader即可：

func (w *ServerWorker) handleStatusEnd(meta *FrameMetadata, reader *buf.BufferedReader) error {
	if s, found := w.sessionManager.Get(meta.SessionID); found {
		...
		common.Interrupt(s.input)
		s.Close()
	}
	...
	return nil
}

相应的，commom/mux/client.go文件下的handleStatusEnd函数也需要做出对应的修改。

@cross-hello
Copy link
Contributor

cross-hello commented Dec 9, 2022 via email

@cross-hello
Copy link
Contributor

cross-hello commented Dec 9, 2022 via email

@renahita6
Copy link
Author

renahita6 commented Dec 9, 2022 •
edited
Loading

Now the code could reduce some elapse time, but still have some remaining, especially for data up.

Thanks for your comment! I have noticed that, but even with "mux: false" there still remain traffic elapsing when uploading. It seems that the reason behind download and upload is different. I am trying to figure it out. But I think this pr can deal with the download situation at least.

@yuhan6665
Copy link
Member

感谢大佬pr！困扰了几代大佬的 MUX 传统艺能就这么给修复了？太强了
有个问题，您提到

在xray中pipe.Reader没有实现Close方法，导致子连接无法正常断开

那么是否应该让 pipe.Reader 实现 Close 呢？

func (r *Reader) Close() error {
	return r.pipe.Close()
}

@renahita6
Copy link
Author

那么是否应该让 pipe.Reader 实现 Close 呢？

这个问题在#285中讨论过了，貌似会引入新的问题。

(btw我不是大佬，你才是大佬)

@yuhan6665
Copy link
Member

@renahita6 嗯 回溯了一下历史讨论然后回到代码 发现了一些有趣的地方
https://github.com/XTLS/Xray-core/blob/main/common/mux/client.go#L217-L219
mux 包里面有好几处这样的代码 在调用 close 的时候同时对 reader interrupt 可以得到以下

  • 这个 reader 不能 close 应该是当年 v 姐的设计
  • 你的修复是对的 我猜测有一些应该 interrupt 的地方 v 姐忘了

现在问题是 除了你加的 handleStatusEnd 之外 mux 包里的其它地方需要不需要？

@renahita6
Copy link
Author

现在问题是 除了你加的 handleStatusEnd 之外 mux 包里的其它地方需要不需要？

这个问题我也思考过，貌似在s.Close前加上对reader的Interrupt总是正确的，但秉承着一个pr修一个bug的原则我没有对其它的s.Close进行修改。此外我还考虑到，只要调用了s.Close那么对应的session一定会被sessionmanager移除，这意味着再次收到另一端对应的session的数据时，sessionmanager会因为找不到接收方而向对方回复End状态 (https://github.com/XTLS/Xray-core/blob/main/common/mux/client.go#L326-L332) ，那最终还是会进入handleStatusEnd函数处理，因此我认为在handleStatusEnd里做出的修改似乎就已经足够了。

@yuhan6665 yuhan6665 merged commit 3e4e050 into XTLS:main Dec 10, 2022
@yuhan6665
Copy link
Member

感谢大佬 先合了 准备发一版让大家测 :)
我感觉如果每个 s.Close 都关掉 reader 的话 就跟 reader 实现 Close 一样了吧？多路复用每个子链接的处理确实比较复杂。。
建议你也给 fly 发个 pr 看看那边的维护者有什么想法

@wenzhuning
Copy link

ray系的mux一直有一个老毛病，取消下载文件后流量还会继续跑，比如#232。 排查后发现问题出在commom/mux/server.go文件下的handleStatusEnd函数。 具体而言，mux客户端向服务端发送End状态请求结束对应的mux.Session时，服务端接受到该请求，并最终进入handleStatusEnd函数：

func (w *ServerWorker) handleStatusEnd(meta *FrameMetadata, reader *buf.BufferedReader) error {
	if s, found := w.sessionManager.Get(meta.SessionID); found {
		...
		s.Close()
	}
	...
	return nil
}

上述if语句中的s.Close函数：

func (s *Session) Close() error {
	common.Close(s.output)
	common.Close(s.input)
	s.parent.Remove(s.ID)
	return nil
}

分别调用s.input和s.output的Close方法，然后将该session的ID从sessionmanager中移除。 问题出现在，s.input是一个pipe.Reader，在xray中pipe.Reader没有实现Close方法，导致子连接无法正常断开，远程服务器会持续通过该pipe.Reader传输数据。 解决方法很简单，在s.Close()前手动中止pipe.Reader即可：

func (w *ServerWorker) handleStatusEnd(meta *FrameMetadata, reader *buf.BufferedReader) error {
	if s, found := w.sessionManager.Get(meta.SessionID); found {
		...
		common.Interrupt(s.input)
		s.Close()
	}
	...
	return nil
}

相应的，commom/mux/client.go文件下的handleStatusEnd函数也需要做出对应的修改。

大佬，对于mux的阻塞问题，是否有方法解决

@devwak devwak mentioned this pull request Dec 17, 2022
Closed
@ZhuangJiayu
Copy link

优雅！特别优雅！大佬给fly那边也提个pr吧。

@zjx20 zjx20 mentioned this pull request Jan 4, 2023
Closed
@rurirei
Copy link
Contributor

rurirei commented Feb 10, 2023 •
edited
Loading

感谢大佬pr！困扰了几代大佬的 MUX 传统艺能就这么给修复了？太强了 有个问题，您提到

在xray中pipe.Reader没有实现Close方法，导致子连接无法正常断开

那么是否应该让 pipe.Reader 实现 Close 呢？

func (r *Reader) Close() error {
	return r.pipe.Close()
}

agreed. for example:

// Close implements common.Closeable.
func (r *Reader) Close() {
	r.pipe.Close()
}

Xray-core/transport/pipe/reader.go

Lines 9 to 27 in 15999e5

// Reader is a buf.Reader that reads content from a pipe.
type Reader struct {
pipe *pipe
}
// ReadMultiBuffer implements buf.Reader.
func (r *Reader) ReadMultiBuffer() (buf.MultiBuffer, error) {
return r.pipe.ReadMultiBuffer()
}
// ReadMultiBufferTimeout reads content from a pipe within the given duration, or returns buf.ErrTimeout otherwise.
func (r *Reader) ReadMultiBufferTimeout(d time.Duration) (buf.MultiBuffer, error) {
return r.pipe.ReadMultiBufferTimeout(d)
}
// Interrupt implements common.Interruptible.
func (r *Reader) Interrupt() {
r.pipe.Interrupt()
}

@cross-hello
Copy link
Contributor

cross-hello commented Feb 10, 2023 via email

@cross-hello
Copy link
Contributor

cross-hello commented Feb 10, 2023 via email

@rurirei rurirei mentioned this pull request Feb 10, 2023
Closed
@rurirei
Copy link
Contributor

rurirei commented Feb 10, 2023

The actual behavior after modifying code in the way don't repair upload flow persistent for some seconds, and also make download flow delay more amount than merged code do. Feb 10, 2023 08:04:50 rurirei @.***>:
…
感谢大佬pr！困扰了几代大佬的 MUX 传统艺能就这么给修复了？太强了 有个问题，您提到 在xray中pipe.Reader没有实现Close方法，导致子连接无法正常断开 那么是否应该让 pipe.Reader 实现 Close 呢？ *func (r *Reader) Close() error { return r.pipe.Close() } * agreed. — Reply to this email directly, view it on GitHub[#1417 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AKGBAYG5FUMYZBXPYKLTYZ3WWWA2DANCNFSM6AAAAAASY4EKNI]. You are receiving this because you commented.[Tracking image][https://github.com/notifications/beacon/AKGBAYESOZHGNWBKC4TXMMTWWWA2DA5CNFSM6AAAAAASY4EKNKWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTSU57CAK.gif]

try #1638

@cross-hello
Copy link
Contributor

cross-hello commented Feb 10, 2023 •
edited
Loading

Modify code in server and client according to pr. ( Have checked code again, and timestamp of generated compilied file)
no, upload will last more than eleven seconds after process finish, and eight seconds for download.

@rurirei
Copy link
Contributor

rurirei commented Feb 10, 2023

Modify code in server and client according to pr. ( Have checked code again, and timestamp of generated compilied file) no, upload will last more than eleven seconds after process finish, and eight seconds for download.

could you try github actions arti https://github.com/XTLS/Xray-core/actions/runs/4141584865/jobs/7161323006

@cross-hello
Copy link
Contributor

Sorry, code doesn't do its duty.

@cross-hello
Copy link
Contributor

cross-hello commented Feb 10, 2023 •
edited
Loading

(Download time elapse excess about two seconds, but upload will take for five)

@yyiu yyiu mentioned this pull request Apr 6, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@renahita6 @cross-hello @yuhan6665 @wenzhuning @ZhuangJiayu @rurirei