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

Routing: Add mutex for Attributes temporarily #3908

Merged
merged 2 commits into from
Oct 15, 2024
Merged

Routing: Add mutex for Attributes temporarily #3908

merged 2 commits into from
Oct 15, 2024

Conversation

Fangliding
Copy link
Member

@Fangliding Fangliding commented Oct 12, 2024

#3904
mmm似乎在inbound上遇到了类似的问题 这里的问题是content
问题还是老问题 对于一条被mux的连接 多个子请求在内部共享一条ctx
mmm的做法是在mux阶段创建一个深拷贝避免它们相互影响
这里更麻烦一些 因为content是在mux后面的dispatcher才被创建的 没法在mux处理 我也拿不定怎么搞 或许WithCancel另起一个新的ctx? 不是很敢乱动
说回这个issue 本质是多个连接都在尝试往content写入数据 当同时写入的时候遇到竞争就炸了 临时解决办法是加锁 这样好歹不会崩溃 但是并没有解决多个连接共享content的问题 这可能导致预期外的路由或者其他非规定行为

@mmmray
Copy link
Collaborator

mmmray commented Oct 12, 2024

ok, this is #3718 but for client?

@Fangliding
Copy link
Member Author

Fangliding commented Oct 12, 2024

ok, this is #3718 but for client?

For server, too
And this fix nothing, just prevents getting panic

@RPRX
Copy link
Member

RPRX commented Oct 13, 2024

既然知道了会有 race condition,加个锁就行了,还加 error log 是干嘛

@Fangliding
Copy link
Member Author

Fangliding commented Oct 13, 2024

既然知道了会有 race condition,加个锁就行了,还加 error log 是干嘛

因为它们本来就不应该尝试往一个map写入 这个修改等于不炸了 换成一个log警告 如果嫌多可以换到debug级别

@RPRX
Copy link
Member

RPRX commented Oct 13, 2024

看了下,所以加锁是治标不治本

直接治本吧,本来就不应该共享同一个 content

这里更麻烦一些 因为content是在mux后面的dispatcher才被创建的 没法在mux处理 我也拿不定怎么搞 或许WithCancel另起一个新的ctx? 不是很敢乱动

你改一下我看看

@RPRX
Copy link
Member

RPRX commented Oct 15, 2024

同样这个也打算先合了,治一下标,晚点再研究治本

@RPRX RPRX changed the title Routing: Add mutex for attr Routing: Add mutex for Attributes temporarily Oct 15, 2024
@RPRX RPRX merged commit 8625753 into main Oct 15, 2024
35 of 36 checks passed
leninalive pushed a commit to amnezia-vpn/amnezia-xray-core that referenced this pull request Oct 29, 2024
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