wy471x commented on PR #6939:
URL: https://github.com/apache/shenyu/pull/6939#issuecomment-6173218401
Closing this, as the bug is already fixed on `master` by #7539 (`1b3baa856`
— *fix(mqtt): fix duplicate startup and resource leaks on config updates*).
`MqttBootstrapServer` on `master` now covers everything this PR set out to
do:
- **failed-start NPE**: `start()` tracks a `boolean started` flag and runs
`finally { if (!started) { shutdown(); } }`, so the event-loop groups are
always released when `bind()` fails.
- **null-safe shutdown**: `shutdown()` checks `future` / `bossGroup` /
`workerGroup` with `Objects.nonNull` *and* clears them, so it is safe to call
at any time, including before any start.
- **swallowed `InterruptedException`**: gone — the interrupt flag is
restored and an `IllegalStateException` is rethrown.
- `start()` / `shutdown()` are `synchronized` and `start()` short-circuits
through `isRunning()`.
Upstream also added regression tests for exactly these paths, and they rely
on `start()` returning synchronously — which is what this PR changes by moving
the bind into `CompletableFuture.runAsync(...)`:
- `failedBindShouldReleaseEventLoopsAndAllowRetry`
- `repeatedStartShouldKeepTheSameResources`
- `concurrentStartsShouldKeepTheSameResources`
- `startShouldReplaceClosedListenerAndReleaseOldEventLoops`
Making `start()` async while it is also `synchronized` would additionally
defeat the lock, since the monitor is released as soon as the task is submitted
— so I'd rather not push that design upstream.
The only part of this PR still valid on `master` is logging: `init()` keeps
the `//// todo log` placeholder (`MqttBootstrapServer.java:52`). If that is
worth picking up I'll send it as a small standalone PR.
Since the fix has landed, **#6747 can be closed as fixed by #7539**.
--
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]
