Clash: 关于 UDP 处理的一些问题

[eycorsican]

1. https://github.com/Dreamacro/clash/blob/48a2013d9c8985e5a0d78133852f048b387044b4/proxy/socks/udp.go#L41 一个 UDP 包一个 goroutine 是很昂贵的，完全没必要

2. https://github.com/Dreamacro/clash/blob/48a2013d9c8985e5a0d78133852f048b387044b4/proxy/socks/udp.go#L41 同样是这里，传进去的数据，如果不立即处理，必须做 copy

3. https://github.com/Dreamacro/clash/blob/48a2013d9c8985e5a0d78133852f048b387044b4/tunnel/tunnel.go#L93 同样用 goroutine 很昂贵

4. https://github.com/Dreamacro/clash/blob/48a2013d9c8985e5a0d78133852f048b387044b4/tunnel/tunnel.go#L162 如果上面坚持用 goroutine，这里必须有锁

5. https://github.com/Dreamacro/clash/blob/48a2013d9c8985e5a0d78133852f048b387044b4/tunnel/tunnel.go#L162 同样是这里，如果用这个 net.Addr 做 key，pc 将永远是 nil，因为那是个内存地址

6. https://github.com/Dreamacro/clash/blob/48a2013d9c8985e5a0d78133852f048b387044b4/tunnel/tunnel.go#L178 UDP 的每个包都可以有不同的 dst addr，要不在 nat 表里把 src 和 dst 同时拿来做 key 做 symmetric nat，不然这个实现有点不伦不类

[xjasonlyu]

感谢大佬指出问题😂，有几个地方没明白：
4.为什么要加锁呢
5.net.Addr是interface{}，在这里net.Addr==*net.UDPAddr==指针，作为key在go里是可以的吧
6.写的时候没考虑nat类型，其实只是参考了go-shadowsocks2的实现，对local UDPConn->remote UDPConn做映射的意思

[mokitoo]

想讨论下关于第一个问题，一个 UDP 包一个 goroutine 是很昂贵，是指内存方面开销还是？
不知你的意思是否是采用epoll来处理请求，如果是epoll的话，go内部其实已经实现了，自己再写一套轮子可能性能还没有go内部调度来的高。
如果没记错的话一个goroutine开销大概是2.5kb，并发不是特别高的话，其实应该还好吧。
毕竟现在内存条也白菜价了。。。。

[xjasonlyu]

想讨论下关于第一个问题，一个 UDP 包一个 goroutine 是很昂贵，是指内存方面开销还是？
不知你的意思是否是采用epoll来处理请求，如果是epoll的话，go内部其实已经实现了，自己再写一套轮子可能性能还没有go内部调度来的高。
如果没记错的话一个goroutine开销大概是2.5kb，并发不是特别高的话，其实应该还好吧。
毕竟现在内存条也白菜价了。。。。

@Buffer2Disk 应该不是这个意思，这里不是 goroutine 内部实现怎么样，而是这里 https://github.com/Dreamacro/clash/blob/48a2013d9c8985e5a0d78133852f048b387044b4/proxy/socks/udp.go#L41
不需要 goroutine 来处理，因为整个func就不会阻塞，直接处理完继续for循环就好了，的确不需要浪费一次 goroutine。（即使 goroutine 开销真的很小）

[eycorsican]

@xjasonlyu

1. 如果使用 goroutine，同一个 udp session 中的多个 UDP 包会同时进入 natTable.Get() 这一步，但这时距离它们任何一个 goroutine 做 natTable.Set() 还有一段距离，导致这些个 goroutine 全部会先返回 nil，再各自去 dial 它们自己的 conn，然后它们各自再做 Set()，然后最后一个做 Set() 的决定后续 UDP 包使用哪个 conn，这时之前几个 conn 全都会无效，所发出的 udp 包的 src addr 也会全乱套，因为各自 dial 的 conn 都会有不同的 src 端口。

1. 问题是同一个 udp session 中，所有 UDP 包都会给一个不同的 localConn.RemoteAddr() object，它们的内存地址都不一样（就算它们的内容一样），你自己打印下吧 :)

2. 我看 go-shadowsocks2 是完美实现 full cone 的，clash 这里的实现则并不是。

[xjasonlyu]

🙊好的知道了，我等下改改。_(:_」∠)_ 多谢大佬指点

[eycorsican]

@Buffer2Disk 我并不是讨论并发问题，是一个 UDP 消耗一个 goroutine 的问题，这里的问题不是并发，更多是合理性问题。另一方面，如果 UDP 数据量超大，则很自然成了性能问题，想像下 1MBps 的速率传 UDP，分成 1500 bytes 一个包，一秒种需要多少个新的 goroutine？

[mokitoo]

@Buffer2Disk 我并不是讨论并发问题，是一个 UDP 消耗一个 goroutine 的问题，这里的问题不是并发，更多是合理性问题。另一方面，如果 UDP 数据量超大，则很自然成了性能问题，想像下 1MBps 的速率传 UDP，分成 1500 bytes 一个包，一秒种需要多少个新的 goroutine？

我以为是按照请求来的，如果是按照字节来的话，那确实不能这么写

[eycorsican]

其实 1 的问题，可以直接把 goroutine 取消掉，不需要其它任何修改，因为后面接了一个 channel。

3 的问题，我看有点复杂，对于 UDP，3 那里当然也可以直接去掉 goroutine，但后面接着还有规则匹配等操作，会不会对传输性能产能影响不好说，没读太仔细，但最好重构下，别这样每个 UDP 包都搞个 goroutine。

[xjasonlyu]

是的，1直接去掉就好了。3可能需要考虑一下怎么解决比较好。

[Dreamacro]

@eycorsican

关于问题 2，如果将 buffer 的取出放到 for 中，有一个地方其实很难处理：关于怎么归还从 pool 取出的 buffer。一般从 pool 取出、归还 buffer 最好是不跨文件使用，但由于某些原因在这里没法在同一个地方取出并归还 buffer，需要在 tunnel 中归还，这其实增加了代码所需要承受的心智负担。

关于 UDP 的问题其实我也在实现 TUN 的时候有做一些修改，包括更好的区分 UDP 与 TCP 流量；3 的 goroutine 是不能取消的，3 是非阻塞接收信息的 root，至于使用 goroutine 处理 UDP 的行为我持保留意见，如果测试之后真的 goroutine 开销十分巨大，再做修改也不迟。

[xjasonlyu]

@eycorsican

关于问题 2，如果将 buffer 的取出放到 for 中，有一个地方其实很难处理：关于怎么归还从 pool 取出的 buffer。一般从 pool 取出、归还 buffer 最好是不跨文件使用，但由于某些原因在这里没法在同一个地方取出并归还 buffer，需要在 tunnel 中归还，这其实增加了代码所需要承受的心智负担。

关于 UDP 的问题其实我也在实现 TUN 的时候有做一些修改，包括更好的区分 UDP 与 TCP 流量；3 的 goroutine 是不能取消的，3 是非阻塞接收信息的 root，至于使用 goroutine 处理 UDP 的行为我持保留意见，如果测试之后真的 goroutine 开销十分巨大，再做修改也不迟。

@Dreamacro 不是吧，我看了下，2中的 buf 是可以放到 for 里面的，因为 buf 会在 handleSocksUDP 这里使用然后由该函数产生新的 buf ，并不会到 tunnel 里去。（不过意义并不大）
https://github.com/Dreamacro/clash/blob/48a2013d9c8985e5a0d78133852f048b387044b4/proxy/socks/udp.go#L58

UDP的解决我想到了比较好的方法，在 tunnel 的 Add 方法里对 TCP UDP 进行区分，这样就不会影响到TCP，而 UDP 在新的 handler 里根据条件是否需要使用新的 goroutine。我已经快改完了，等一下PR。

[Dreamacro]

@xjasonlyu 对 buffer 的切片并不是一个全新的 buffer

[xjasonlyu]

@Dreamacro 是的吧，socks5.DecodeUDPPacket(packet)会对 buf 进行

payload = bytes.Join([][]byte{packet[3+len(addr):]}, []byte{})

操作，而bytes.Join里又会

return append([]byte(nil), s[0]...)

就相当于clone了一个buffer

[Dreamacro]

@xjasonlyu emmm, 明明只有一个元素，为什么要用 Join 这种方法 hack 去浅拷贝

[xjasonlyu]

我刚才的意思是说把

go handleSocksUDP(l, buf[:n], remoteAddr) 

的 go 去掉后，因为这里用goroutine是真的没有必要

@xjasonlyu emmm, 明明只有一个元素，为什么要用 Join 这种方法 hack 去浅拷贝

是啊，当时看到上面的都是用Join的，也就没多想直接Join了😓

[eycorsican]

基本上把这里 https://github.com/eycorsican/Mellow/issues/4 提到的问题处理好，最起码的 symmetric nat 类型 udp 流量处理也就没太大问题，其它性能之类的问题也就顺便提一下，你们讨论我溜了。

[xjasonlyu]

@Dreamacro 我在 https://github.com/Dreamacro/clash/pull/265 初步解决了一下上面提到的UDP问题。

[Dreamacro]

maybe fixed in latest dev