btlqql opened a new pull request, #5352:
URL: https://github.com/apache/rocketmq-dashboard/pull/5352
### Which Issue(s) This PR Fixes
- Fixes #5057
### Problem
`ProxyConsumerResolver.remotingClient()` marked the shared Netty remoting
client as started
*before* calling `start()`, and it published the client object before the
start had run at all:
```java
private NettyRemotingClient remotingClient() {
NettyRemotingClient client = remotingClient;
if (client == null) {
synchronized (this) {
if (remotingClient == null) {
remotingClient = new NettyRemotingClient(new
NettyClientConfig());
}
client = remotingClient;
}
}
if (clientStarted.compareAndSet(false, true)) {
client.start();
}
return client;
}
```
Two consequences follow from that order:
- A first `start()` that throws leaves both the client object and
`clientStarted=true` behind, so
every later proxy query skips `start()` and keeps using the client that
never finished
initialising. Proxy consumer diagnostics stay "unavailable" for the whole
process lifetime, even
after the transient startup problem is gone.
- A second caller can observe `clientStarted=true` while the first caller's
`start()` is still
running and pass that half-initialised client to `invokeSync`.
`shutdownRemotingClient()` also keyed off the same flag, so a candidate
whose start failed was
neither published correctly nor disposed of.
### Changes
`server/src/main/java/org/apache/rocketmq/studio/provider/apache/ProxyConsumerResolver.java`
- The client is now published only after `start()` returned. The
`AtomicBoolean clientStarted` flag
is gone: being published *is* the started state, so there is no window in
which a client is
visible while its startup is still running.
- Initialisation happens under the resolver monitor (lock-free read on the
published field first),
so concurrent first-use callers wait for the startup instead of racing it.
- A `start()` that throws disposes of the candidate
(`shutdownFailedCandidateQuietly`) and rethrows;
nothing is published, so the next request creates and starts a fresh
client.
- `shutdownRemotingClient()` takes the same monitor, so shutdown cannot run
while a startup is in
flight, and it only ever shuts down a client that was actually started.
`server/src/test/java/org/apache/rocketmq/studio/provider/apache/ProxyConsumerResolverTest.java`
-
`firstStartFailureShouldNotBeCachedAndTheNextQueryShouldStartAFreshClientTest`
— the first client's
`start()` throws, the failed candidate must be shut down and must not be
reused, and the next
query must create and start a replacement.
- `concurrentFirstUseShouldWaitUntilTheRemotingClientStartupFinishedTest` —
the second caller arrives
while the first caller is still inside `start()`; it must not reach
`invokeSync` before the
startup finished, and only one client may be constructed.
### How Did You Test This Change?
Red (test file only, production source unmodified at `6a68042f`):
```
cd server && mvn -B -ntp test -Dtest=ProxyConsumerResolverTest
[ERROR] Tests run: 8, Failures: 2, Errors: 0, Skipped: 0
[ERROR]
ProxyConsumerResolverTest.firstStartFailureShouldNotBeCachedAndTheNextQueryShouldStartAFreshClientTest:214
Wanted but not invoked: nettyRemotingClient.shutdown();
[ERROR]
ProxyConsumerResolverTest.concurrentFirstUseShouldWaitUntilTheRemotingClientStartupFinishedTest:271
expected: null but was: java.lang.IllegalStateException: proxy
query reached the client before startup finished
```
Green (same worktree with the fix applied):
```
cd server && mvn -B -ntp test
-Dtest=ProxyConsumerResolverTest,RocketMQConsumerDiagnosticsProviderTest,RocketMQMetadataProviderTest,RocketMQAdminClientImplTest
[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0 -- in
...ProxyConsumerResolverTest
[INFO] Tests run: 80, Failures: 0, Errors: 0, Skipped: 0 -- in
...RocketMQAdminClientImplTest
[INFO] Tests run: 11, Failures: 0, Errors: 0, Skipped: 0 -- in
...RocketMQConsumerDiagnosticsProviderTest
[INFO] Tests run: 46, Failures: 0, Errors: 0, Skipped: 0 -- in
...RocketMQMetadataProviderTest
[INFO] Tests run: 145, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
[INFO] You have 0 Checkstyle violations.
```
Both runs execute Checkstyle at `validate` with 0 violations. This is a
focused backend run, not the
full suite (the suite has pre-existing failures on this branch that need
MySQL 8 / external tooling).
### Overlap disclosure
An independent PR for the same issue already exists: #5255 (its predecessor
#5058 was closed by the
inactivity bot). This change is an independent implementation written from
the issue and the current
source; the overlapping part is the intent both share (publish only after a
successful start and
retry with a fresh client). The differences are: the started state here is
the publication itself
rather than a separate boolean flag, `shutdownRemotingClient()` is
serialised with initialisation,
and the two regressions drive the interleavings through Mockito construction
mocking instead of a
change to the production surface. The issue is still open and the fix is
still absent from
`rocketmq-studio`, which is what the red run above shows.
### Checklist
- [x] One coherent bug fix; no unrelated changes
- [x] Conventional Commit subject
- [x] Startup-failure/retry and concurrent-first-use regressions
- [x] No API contract change, no new UI text, no new files
- [ ] Full CI is green
--
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]
