lymerin commented on PR #7539: URL: https://github.com/apache/shenyu/pull/7539#issuecomment-6107222236
> Reviewed locally at [4917d90](https://github.com/apache/shenyu/commit/4917d90cfa8e73dbd93a168ba221e578a6a3716c) (JDK 21, offline Maven). Ran the full module suites for this change: `shenyu-protocol-mqtt` 87 tests + `shenyu-plugin-mqtt` 16 tests (`MqttPluginDataHandlerTest`), 103/103 green, checkstyle clean. CI on the head commit is green. > > The fix is correct and the direction is right: serializing updates, replacing the port probe with real lifecycle state, comparing the _effective_ configuration, and cleaning up event-loop groups on both restart and failed start removes the leaked-group / double-bind path. Two remarks, both non-blocking: > > 1. `MqttServerConfiguration#equals`/`hashCode` call `effectivePassword()`, which is deterministic (`EncryptUtil.choose` is a plain MD5 hex), so equality and the hash are stable — good. However `leakDetectorLevel.toUpperCase(Locale.ROOT)` throws NPE when the field is null (a config such as `{"leakDetectorLevel":null}` deserialized from `PluginData.config`). `MqttBootstrapServer#start` has the same pre-existing exposure. Normalizing null-safely (e.g. `StringUtils.defaultString(leakDetectorLevel)`) would close it. > 2. Comparing only the runtime-applied subset inside `equals`/`hashCode` is a surprising contract for a mutable configuration object (two instances differing in `bossGroupThreadCount` or `encryptMode` are "equal", which also silently breaks anyone putting these into a `Set`/`Map`). A named method such as `hasSameRuntimeSettings(MqttServerConfiguration)` would say what the handler actually means. Related observation from the same read: `afterPropertiesSet()` never pushes `bossGroupThreadCount` into `MqttContext`, so `ENV.getBossGroupThreadCount()` is always `0` and Netty picks the default thread count — pre-existing, but it makes the field dead weight today. > > Nice touch that `start()` now releases the groups when binding fails, and that the tests cover failed start / retry / closed-listener recovery instead of only the happy path. Thanks for the detailed review. I updated the comparison logic based on both remarks: - Replaced equals() / hashCode() with hasSameRuntimeSettings() so the method expresses the actual intent and does not redefine general object equality. - Made leakDetectorLevel comparison null-safe while keeping the existing case-insensitive runtime comparison. The MQTT lifecycle behavior and compared runtime fields remain unchanged. I also kept the pre-existing bossGroupThreadCount behavior out of this PR. Could you take another look when convenient? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
