Skip to content

Timer: prevent creating redundant check task, Splice: prevent set timeout when UplinkOnly/DownlinkOnly timeout is already set#5046

Closed
patterniha wants to merge 3 commits intomainfrom
timer-redundant
Closed

Timer: prevent creating redundant check task, Splice: prevent set timeout when UplinkOnly/DownlinkOnly timeout is already set#5046
patterniha wants to merge 3 commits intomainfrom
timer-redundant

Conversation

@patterniha
Copy link
Collaborator

@patterniha patterniha commented Aug 22, 2025

requestDone := func() error {
		defer timer.SetTimeout(plcy.Timeouts.DownlinkOnly)
                ...
}

responseDone := func() error {
		defer timer.SetTimeout(plcy.Timeouts.UplinkOnly)
                ...
}

currently, after one timeout is set, another one will be set after timeout-exceed and function-return, but it is redundant.

//////////////////////////////////////////////////////////////////////////////////////////////

also, if UplinkOnly/DownlinkOnly timeout is already set , splice-timeout should not be set, and I will fix it soon.

Xray-core/proxy/proxy.go

Lines 599 to 602 in 33272a0

timer.SetTimeout(8 * time.Hour) // prevent leak, just in case
if inTimer != nil {
inTimer.SetTimeout(8 * time.Hour)
}

@patterniha
Copy link
Collaborator Author

also, if UplinkOnly/DownlinkOnly timeout is already set , splice-timeout should not be set, and I will fix it soon.

done.

@patterniha patterniha changed the title Timer: prevent creating redundant check task Timer: prevent creating redundant check task, Splice: prevent set timeout when UplinkOnly/DownlinkOnly timeout is already set Aug 22, 2025
@Fangliding
Copy link
Member

Fangliding commented Aug 22, 2025

overridden参数太奇怪了 一个ActivityTimer应该就不存在设置之后无法触发onTimeout的情况除非被故意设置得非常长 这里防止leak的话按目的应该改成 SetTimeoutShorter 或者类似的东西 检查checkTask(如果存在)的interval大于设定的值就重置 否则不重置

@patterniha
Copy link
Collaborator Author

patterniha commented Aug 22, 2025

@Fangliding

splice-timeout is 8-hours, it is greater than both connIdle(5 min), and UplinkOnly/DownlinkOnly(2-5 sec).

First, timer set to connIdle and if one-side is disconnected, it becomes UplinkOnly/DownlinkOnly.

but only when timeout is connIdle, we can change it to 8 hours, and if one-side is already disconnected we should not change the timer-timeout.

///

so, if timeout is changed by UplinkOnly/DownlinkOnly(=overridden), we should not set splice-timeout to 8 hours, otherwise we can set.
so we need SetTimeoutIfNotOverridden.

///

also, in splice, we change the timeout to 8 hours, because we can't update timer when splice, so we set it to long enough value, i change it to 24 hours(default TCP NAT timeout)

@Fangliding
Copy link
Member

哦我误解了注释里的prevent leak

@patterniha
Copy link
Collaborator Author

patterniha commented Aug 23, 2025

@Fangliding changes timer codes, so i rebase this pr, after that.
or @Fangliding it is better to add "overriden" to your code and fix this problem too, then i close this pr.

@Fangliding
Copy link
Member

Fangliding commented Aug 24, 2025

我不确定这是否正确 重新读了一遍我觉得这甚至应该是错的
这里这么操作的原因是splice不执行一般的 io 操作导致无法在每次copy的时候都执行对timer的更新 设置成8小时的作用是完全禁用超时操作 执行uplink/downlinkonly的操作后不是两三秒内就关掉 是把需要update的时间缩短到这么短 如果在半关闭后仍有一端在持续传输那么这个连接是完全可以活下去的 你这么做会导致uplink关闭两三秒后downlink必定关闭 这是错误的

@patterniha
Copy link
Collaborator Author

If one end continues transmitting after a half-close, the connection can survive. Doing this will cause the downlink to close two or three seconds after the uplink closes, which is incorrect.

? usually DownlinkOnly-timeout is set after splice-8-hours-timeout, and this makes splice-reading closed even if we have continues-transmitting, this is another problem.

@patterniha
Copy link
Collaborator Author

patterniha commented Aug 25, 2025

anyway, for my use, i set uplinkOnly/downlinkOnly to 0(in serverless), So that i don't encounter this problem.
I'll check it again later.

@patterniha patterniha closed this Aug 25, 2025
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.

2 participants