Refactor StatSvc ip recording, fix #2349 #2351

sandtechnology · 2022-11-17T17:32:24Z

I'm the beginner for C language, so feel free to let me know if there have any wrong here : D

Changes:

Getting server ip from socket directly.
Change windows native socket code, only use ipv4 instead and fix unsafe printf usage.
Fix wrong bid used in StatSvc.offline.

Him188 · 2022-11-17T17:43:39Z

请添加单元测试，至少测试不需要网络请求的 ipv4

mirai-core/src/jvmBaseMain/kotlin/network/protocol/packet/login/StatSvc.kt

Him188 · 2022-12-04T21:06:38Z

test 失败了

Fallback to resolve dns when testing

Using all instead of any

Him188 · 2022-12-08T12:15:28Z

mirai-core/src/commonTest/kotlin/network/framework/AbstractCommonNHTestWithSelector.kt

@@ -30,14 +30,15 @@ internal abstract class AbstractCommonNHTestWithSelector :
        overrideComponents[BotOfflineEventMonitor] = BotOfflineEventMonitorImpl()
    }

-    val conn = PlatformConn()
+    val conn get() = PlatformConn(createAddress())


Him188 · 2022-12-08T12:17:47Z

mirai-core/src/jvmBaseMain/kotlin/network/impl/netty/NettyNetworkHandler.kt

@@ -131,6 +131,16 @@ internal open class NettyNetworkHandler(
        return contextResult.await()
    }

+    override fun io.netty.channel.Channel.getConnectedIP(): Long = this.remoteAddress().let { address ->


为什么要有这个lambda？

Him188 · 2022-12-08T12:18:15Z

mirai-core/src/jvmBaseMain/kotlin/network/impl/netty/NettyNetworkHandler.kt

+                0L
+            }
+        }
+    }.invoke()


创建lambda立即invoke只会增加复杂度

Him188 · 2022-12-08T12:19:29Z

mirai-core/src/jvmBaseTest/kotlin/network/framework/AbstractCommonNHTest.kt

@@ -51,7 +57,16 @@ internal actual abstract class AbstractCommonNHTest actual constructor() :
        }
    }

-    actual val conn: PlatformConn = NettyNHTestChannel()
+    actual val conn: PlatformConn get() = PlatformConn(address = createAddress())


JVM那边的测试也是get() 而且这里是创建连接才会去get这个 get()这里应该没问题

Him188 · 2022-12-08T12:24:29Z

mirai-core/src/nativeTest/kotlin/network/framework/AbstractCommonNHTest.kt

@@ -32,8 +37,22 @@ internal actual abstract class AbstractCommonNHTest actual constructor() :
    protected actual fun removeOutgoingPacketEncoder() {
    }

-    actual val conn: PlatformConn = PlatformConn()
+    actual val conn: PlatformConn get() = PlatformConn(createAddress())



JVM那边的测试也是get() 而且这里是创建连接才会去get这个 get()这里应该没问题

属性get有副作用而且每次都创建新实例是不好的

Karlatemp · 2022-12-08T15:14:08Z

mirai-core/src/commonMain/kotlin/network/protocol/packet/login/StatSvc.kt

+                        uOldSSOIp = if (lastDisconnectedIP in 1..2) {
+                            -lastDisconnectedIP
+                        } else {
+                            lastDisconnectedIP
+                        }
+                        uNewSSOIp = if (lastConnectedIP in 1..2) {
+                            -lastDisconnectedIP
+                        } else {
+                            lastConnectedIP
+                        }


这部分应该在提供这个 ip 的地方处理

Karlatemp · 2022-12-08T15:16:03Z

mirai-core/src/commonMain/kotlin/utils/PlatformSocket.kt

@@ -24,6 +24,7 @@ import kotlin.jvm.JvmName
 */
 internal expect class PlatformSocket : Closeable, HighwayProtocolChannel {
    val isOpen: Boolean
+    val connectedIp: Long


给特殊意义值留下注释

Karlatemp · 2022-12-08T15:17:30Z

mirai-core/src/mingwX64Main/kotlin/utils/PlatformSocket.kt

+    actual val connectedIp: Long
+        get() = if (isOpen) {
+            sockets.socket_get_connected_ip(socket).toLong()
+        } else {
+            0L
+        }


应该在这里就直接处理特殊值 (0, 1, ....) 的情况(即切换回负数), 而不是交给外部

AdoptOSS · 2022-12-29T04:21:26Z

Change windows native socket code, only use ipv4 instead and fix unsafe printf usage.

Why? This should not be applied without special reasons.

sandtechnology · 2022-12-29T04:23:49Z

Change windows native socket code, only use ipv4 instead and fix unsafe printf usage.

Why? This should not be applied without special reasons.

Because in my test it returning ipv6 first and cause connect failed lol

AdoptOSS · 2022-12-29T04:26:10Z

You should recheck your network environment first. Even if there is a bug, it’s not wise to disable Ipv6 rather than fixing the bug.

sandtechnology · 2022-12-29T04:32:46Z

Change windows native socket code, only use ipv4 instead and fix unsafe printf usage.

Why? This should not be applied without special reasons.

Because in my test it returning ipv6 first and cause connect failed lol

You should recheck your network environment first. Even if there is a bug, it’s not wise to disable Ipv6 rather than fixing the bug.

But I just have ipv6 in my local network and could not use ipv6 to connect, it's the most user environment (in China), and currently tencent just provide ipv4 address and a domain, in this case, original native code will be resulted in could not init connection and retrying (we just have this domain in ip list when first init)

sandtechnology · 2022-12-29T04:38:34Z

If you want to use ipv6 anyway, linux code need to refactor as well since it just support ipv4, and both need to impl a dns query then use a loop to try every ip

AdoptOSS · 2022-12-29T04:44:28Z

it's the most user environment (in China)

I don't think so, at least almost all ISPs in maintain China begin to distribute IPv6 addresses.

and currently tencent just provide ipv4 address and a domain

This is a powerful point. I'll check it later.

original native code will be resulted in could not init connection and retrying

No, the origin code creates a dual-stack socket (via ipv6only=0). It should not retry if ipv6 is not avaliable.

both need to impl a dns query then use a loop to try every ip

I don't think so. WSAConnectByNameA should try any possible address and choose a best internally.

AdoptOSS · 2022-12-31T09:22:36Z

I’ve tried to disable IPv4 totally and then open Mobile QQ, it works well. I think we need to dig into it.

Him188 · 2023-02-19T12:00:17Z

我觉得可以先完善 v4, 之后再考虑 v6

Turning toIpV4Long function into MMP, fix mamoe#2349

03d3ad8

sandtechnology added 3 commits November 18, 2022 02:04

Add unit test for ip conversion

aa846a5

Fix bug and add test to unit test

a89da69

Fix offline bid

40516d9

Karlatemp reviewed Nov 17, 2022

View reviewed changes

mirai-core/src/jvmBaseMain/kotlin/network/protocol/packet/login/StatSvc.kt Outdated Show resolved Hide resolved

Karlatemp linked an issue Nov 18, 2022 that may be closed by this pull request

java.lang.NumberFormatException: Invalid number format: 'msfwifi' #2349

Open

Him188 added this to the 2.14.0-RC milestone Nov 20, 2022

sandtechnology added 9 commits December 6, 2022 15:59

Refactor to get socket ip directly

040bc16

Fallback to resolve dns when testing

Fix bug in getConnectedIPPlatform

68a4958

Using all instead of any

Fix build

54cd19b

Fix unit test

0fb8886

Fix build

e831be6

Fix unit test and build

6a07617

Fix build

df762ce

Add ws2_32.lib to linker

fe8b1c9

Add Dnsapi to linker

e5707ed

sandtechnology changed the title ~~Turning toIpV4Long function into MMP, fix #2349~~ Refactor StatSvc ip recording, fix #2349 Dec 6, 2022

sandtechnology and others added 3 commits December 6, 2022 20:56

Try to use -l instead of -L

b91e4e5

Fix missed space in linked opts

7a61f04

Merge branch 'dev' into dev

bb44c2f

sandtechnology requested a review from Karlatemp December 6, 2022 16:43

Karlatemp requested a review from Him188 December 7, 2022 04:02

Him188 reviewed Dec 8, 2022

View reviewed changes

Karlatemp reviewed Dec 8, 2022

View reviewed changes

AdoptOSS mentioned this pull request Dec 31, 2022

Support IPv6 #2409

Open

Him188 modified the milestones: 2.14.0-RC, 2.15 Jan 2, 2023

Him188 added t:enhancement 类型: 现有功能上的优化 s:core 子系统: mirai-core labels Jan 18, 2023

Him188 modified the milestones: 2.15, 2.15.0-RC Jan 18, 2023

Him188 removed this from the 2.15.0-RC milestone Feb 19, 2023

Him188 marked this pull request as draft March 20, 2023 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor StatSvc ip recording, fix #2349 #2351

Refactor StatSvc ip recording, fix #2349 #2351

sandtechnology commented Nov 17, 2022 •

edited

Him188 commented Nov 17, 2022

Him188 commented Dec 4, 2022

Him188 Dec 8, 2022

Him188 Dec 8, 2022

Him188 Dec 8, 2022

Him188 Dec 8, 2022

sandtechnology Dec 8, 2022

Him188 Dec 8, 2022

sandtechnology Dec 8, 2022

Him188 Dec 8, 2022

Karlatemp Dec 8, 2022

Karlatemp Dec 8, 2022

Karlatemp Dec 8, 2022

AdoptOSS commented Dec 29, 2022

sandtechnology commented Dec 29, 2022

AdoptOSS commented Dec 29, 2022 via email

sandtechnology commented Dec 29, 2022 •

edited

sandtechnology commented Dec 29, 2022

AdoptOSS commented Dec 29, 2022

AdoptOSS commented Dec 31, 2022 via email

Him188 commented Feb 19, 2023

Refactor StatSvc ip recording, fix #2349 #2351

Are you sure you want to change the base?

Refactor StatSvc ip recording, fix #2349 #2351

Conversation

sandtechnology commented Nov 17, 2022 • edited

Him188 commented Nov 17, 2022

Him188 commented Dec 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdoptOSS commented Dec 29, 2022

sandtechnology commented Dec 29, 2022

AdoptOSS commented Dec 29, 2022 via email

sandtechnology commented Dec 29, 2022 • edited

sandtechnology commented Dec 29, 2022

AdoptOSS commented Dec 29, 2022

AdoptOSS commented Dec 31, 2022 via email

Him188 commented Feb 19, 2023

sandtechnology commented Nov 17, 2022 •

edited

sandtechnology commented Dec 29, 2022 •

edited