lizhimins commented on PR #5352:
URL:
https://github.com/apache/rocketmq-dashboard/pull/5352#issuecomment-6094445658
Confirmed against trunk: ProxyConsumerResolver.java:227-231 publishes the
client inside the monitor
and :233-235 CASes + start()s outside it, so a first start() that throws
leaves a permanently
unstarted client behind, and a concurrent first-use caller can hand that
half-initialised client to
invokeSync. Both regressions are genuinely mutation-sensitive (I could name
the reverting line for
each assertion), and every symbol you reference exists — including
mockConstruction, which already
has precedent here in MqClientPoolTest:37,80. Four items before this can
land:
1. shutdownRemotingClient() now contends on the same monitor that
remotingClient() holds *across*
NettyRemotingClient.start(). A hung startup will block Spring's @PreDestroy
indefinitely (there
is no timeout), so the context close stalls until SIGKILL. Use a bounded
ReentrantLock.tryLock,
or keep shutdown off the initialization lock.
2. Failure amplification: resolveConsumerConnectionStatus:96-110 calls
remotingClient() once per
proxy address, and RocketMQMetadataProvider:359 calls it once per consumer
group. With a
persistent start() failure, one group-list request now performs (groups x
proxies) full
construct/start/shutdown Netty lifecycles, on every request, with no
backoff or negative cache.
The old code was broken but cheap. Please add a short backoff or one
attempt per resolve.
3. Narrow shutdownFailedCandidateQuietly's `catch (Exception)` to
RuntimeException and chain the
shutdown failure with addSuppressed on the rethrown cause instead of only
logging it.
4. Wrap the latch awaits in assertThat(...).isTrue() — as written, a latch
that never fires does
not fail fast and the test dies on an unrelated assertion later.
Two smaller notes: `import org.mockito.MockedConstruction;` sits before
`org.mockito.Mock;`, which
checkstyle does not enforce here but does break this file's alphabetical
style; and in the
concurrency test the `events` assertion only holds because both invokeSync
labels are the same
string — the concurrent caller can legitimately win the monitor first, so
please say so in a
comment before someone differentiates those labels and turns it flaky.
Also, one factual correction: on trunk :241 (`clientStarted.get() &&
remotingClient != null`) both
are true after a failed start, so @PreDestroy does shut the candidate down;
what is missing is
*prompt* disposal, i.e. the Netty event loops stay allocated for the rest of
the process lifetime.
--
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]
