lymerin commented on PR #7539: URL: https://github.com/apache/shenyu/pull/7539#issuecomment-6107226701
> ## Review: fix MQTT duplicate startup and resource leaks on config updates > Solid fix for two real bugs: (1) every `handlerPlugin` call re-ran `server.init()`/`server.start()` without tearing down the previous server, leaking NIO event-loop groups and leaving the old listener bound; (2) the `isPortUsing` socket probe was brittle. > > **What changed (and why it's correct)** > > * `MqttPluginDataHandler.handlerPlugin` is now `synchronized` and tracks `runningConfiguration`. On a config update it compares the incoming `MqttServerConfiguration` via a **newly added `equals`/`hashCode`** (comparing only the settings applied to the MQTT context) and skips re-init when the config is unchanged **and** the server is still running. On change/disable it shuts down the previous server (nulling `future`/`bossGroup`/`workerGroup`) before starting the new one. > * `MqttBootstrapServer.start()`/`shutdown()` are now `synchronized`; `shutdown()` nulls `future`/`bossGroup`/`workerGroup` so `isRunning()` reflects reality; `start()` fails cleanly (releasing event loops via a `finally` block) on bind error. The fragile socket-based `isPortUsing` probe is replaced by the `isRunning()` check. > * The field type changed from the `BootstrapServer` interface to the concrete `MqttBootstrapServer` (which carries `isRunning()`). I confirmed the interface has no `isRunning()` and the field is `private`, so there are no external dependents. The added `equals`/`hashCode` don't affect any existing `Map` usage (grep showed none). > > **Verification** > > * `BootstrapServer` interface declares `init/start/shutdown` only — no `isRunning()` — so the concrete-type switch is required and correct. > * CI: full matrix green. The new tests are thorough: enable/disable lifecycle, equivalent-config keeps resources, config-change replaces resources, failed bind retries, **concurrent updates start only once**, closed-listener restart, and a real MQTT client connection that survives repeated updates. > > **Non-blocking note** `MqttServerConfiguration` now implements `equals`/`hashCode`. It's only used as a value here, but if it's ever used as a `Map` key elsewhere, semantics change — a quick grep showed no such usage. Also `MqttContext` global state touched in tests (`setBossGroupThreadCount`) is shared; fine for tests, just be aware. > > Approving — the lifecycle is now idempotent and leak-free, with strong test coverage. Thanks for the approval. I made a small follow-up after the reviews: - Removed the custom equals() / hashCode() from MqttServerConfiguration. - Replaced them with hasSameRuntimeSettings() for the MQTT restart decision. This keeps the runtime comparison behavior unchanged while avoiding surprising equality semantics if the configuration object is ever used in a Map or Set. I also made the leakDetectorLevel comparison null-safe. The lifecycle logic and existing regression coverage are unchanged. CI is rerunning on the updated commit. 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]
