Skip to content

refactor: UdpSocketController to customized UdpSocket#728

Open
huster-zhangpeng wants to merge 1 commit into
mainfrom
refactor/usc
Open

refactor: UdpSocketController to customized UdpSocket#728
huster-zhangpeng wants to merge 1 commit into
mainfrom
refactor/usc

Conversation

@huster-zhangpeng

Copy link
Copy Markdown
Contributor

No description provided.

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 38.61789% with 151 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
qprotocol/src/packet.rs 30.76% 81 Missing ⚠️
qudp/src/ext.rs 0.00% 64 Missing ⚠️
qtraversal/src/packet.rs 25.00% 3 Missing ⚠️
qtraversal/src/punch/puncher.rs 66.66% 3 Missing ⚠️
Files with missing lines Coverage Δ
qbase/src/time.rs 89.92% <ø> (ø)
qbase/src/util/wakers.rs 91.58% <100.00%> (+0.41%) ⬆️
qinterface/src/io/handy.rs 64.59% <100.00%> (+1.13%) ⬆️
qtraversal/src/future.rs 98.57% <100.00%> (ø)
qtraversal/src/route.rs 51.69% <ø> (ø)
qudp/src/lib.rs 71.61% <100.00%> (+0.45%) ⬆️
qtraversal/src/packet.rs 42.73% <25.00%> (ø)
qtraversal/src/punch/puncher.rs 30.20% <66.66%> (ø)
qudp/src/ext.rs 0.00% <0.00%> (ø)
qprotocol/src/packet.rs 30.76% <30.76%> (ø)

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread qbase/src/util/wakers.rs
self.lock_guard().remove(waker)
}

pub fn together_with(self: &Arc<Self>, waker: &Waker) -> Waker {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

这个API很容易用错

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

为啥这么说?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

很容易直接together_with得到Waker然后构造Context传进poll函数
但是后来发现这一个流程是有问题的,在poll操作直接ready而没有pending -> wake -> ready 流程时,会导致有一个waker残留在里面
对于几乎总是直接ready的API(poll send似乎就是此类), 这个问题造成的影响尤为显著

Comment thread qbase/src/util/wakers.rs
) -> Poll<T> {
self.register(cx.waker());
self.add(cx.waker());
let result = poll(&mut Context::from_waker(&self.to_waker()));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

这里怎么不用together_with

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

这里的 combine 函数名,不太像是 Wakers 的内在属性,我其实想换成面向过程的,不太喜欢这个函数名,在对比两种写法

Comment thread qprotocol/src/io.rs
transactions: DashMap<stun::TransactionId, ArcReceiving<stun::Response>>,
// ── NAT feature ─────────────────────────────────────────────────────
/// Per-socket NAT type, resolved asynchronously by the NAT probe task.
nat_features: DashMap<Fd, SetOnce<NetFeature>>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

多agent可能给出不一样的nat feautre, 如何处理

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SetOnce而不是Arc, 这会使得异步等待会持有guard, 导致死锁
dashmap的guard虽然Send也Sync, 但是这往往会导致用错,导致死锁

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

这个文件,很久之前的,暂时还没在 lib.rs 中引用,这是 qinterface 重构的早期,qinterface 逻辑乱着呢,距离完成早着呢,可以先忽略它

Comment thread qprotocol/src/io.rs
/// Per-socket NAT type, resolved asynchronously by the NAT probe task.
nat_features: DashMap<Fd, SetOnce<NetFeature>>,
/// Externally-visible addresses discovered by agents.
endpoint_addrs: DashMap<Fd, HashMap<SocketAddr, Arc<SetOnce<SocketAddr>>>>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

地址变化直接整个替换?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not "Vec because the number of agents per socket is typically tiny."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

今天下午 review 出的结构更好,这里确实不妥。整体都是雏形,此文件没参与编译,所以 CI 才过了,别紧张

Comment thread qprotocol/src/io.rs
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub struct Fd(usize);

static FD_COUNTER: AtomicUsize = AtomicUsize::new(0);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

一般这种逻辑的唯一ID生成器产出的都是从1开始的,如果某次生成取到了0就说明发生了溢出,生成的ID将不再是唯一的。这个没考虑到

Comment thread qprotocol/src/io.rs
stun_addr: agent_addr,
});
let mut agents = self.agents.entry(fd).or_default();
if agents.iter().any(|(a, _)| *a == agent_addr) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

一堆dashmap, 并发问题心智负担太大了,很容易就可以发现到问题
T1: L159
T2: L138...继续往后,ret close
T1: 继续。这时候agents就有了一个不存在的FD作为key

Comment thread qprotocol/src/io.rs
.nat_features
.get(&fd)
.ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "unknown fd"))?;
Ok(*once.wait().await)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

会死锁

Comment thread qprotocol/src/io.rs

/// Remove a previously registered STUN agent.
pub fn del_agent(&self, fd: Fd, agent_addr: SocketAddr) {
if let Some(mut agents) = self.agents.get_mut(&fd) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

和add_agent会有并发问题

Comment thread qprotocol/src/io.rs
.ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "unknown agent"))?
};

Ok(*once.wait().await)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

和close有并发问题。这里可能会永远不会被唤醒

Comment thread qprotocol/src/packet.rs
impl<T: BufMut> WriteStunHeader for T {
fn put_stun_header(&mut self, stun_header: &StunHeader) {
self.put_u8(STUN_HEADER_BITS);
self.put_u32(0);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

magic number

Comment thread qprotocol/src/packet.rs
fn put_forward_header(&mut self, forward_header: &ForwardHeader);
}

impl<T: BufMut> WriteForwardHeader for T {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

这个blanket impl不适配DST

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

这是从 qtraversal 哪里拷贝过来的,别紧张,只是在整理,这个 rs文件同样没被激活使用

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