Skip to content

fix: avoid consuming logs after client init failure - #6959

Merged
Aias00 merged 4 commits into
apache:masterfrom
hengyuss:fix/AbstractLogConsumeClient.initClient_marks_started_even_when_initClient0_failed_NPE_in_consume0
Aug 23, 2026
Merged

fix: avoid consuming logs after client init failure#6959
Aias00 merged 4 commits into
apache:masterfrom
hengyuss:fix/AbstractLogConsumeClient.initClient_marks_started_even_when_initClient0_failed_NPE_in_consume0

Conversation

@hengyuss

Copy link
Copy Markdown
Contributor

Fixes: #6846

Ensure logging clients are marked as started only after successful initialization, preventing log consumption with
uninitialized producers. Add null guards, failure cleanup, and related tests.

Make sure that:

  • You have read the contribution guidelines.
  • You submit test cases (unit or integration tests) that back your changes.
  • Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Fixes #6846 — logging clients were marked started (isStarted=true) even when initClient0 failed, so consume then touched uninitialized producers/sinks and threw NPEs.

  • AbstractLogConsumeClient.initClient0 return type changed from void to boolean; initClient now sets isStarted to the init result and only registers the shutdown hook when initialization succeeded.
  • All 9 concrete clients (Aliyun SLS, ClickHouse, ElasticSearch, Huawei LTS, Kafka, Pulsar, RabbitMQ, RocketMQ, Tencent CLS) updated: return false on blank-config / failure (with best-effort close0() cleanup where the producer/channel was partially created), true on success.
  • Added null guards in consume0 for Kafka (producer), Pulsar (producer), RabbitMQ (channel), RocketMQ (producer), Tencent CLS (client) — skip publishing with a warning instead of NPE.
  • Pulsar close0 restructured so client.close() runs even if producer is null.
  • New AbstractLogConsumeClientTest covers not-started-skip, started-runs, and failed-reinit leaves client stopped.

Verification

  • Compile safety: grepped extends AbstractLogConsumeClient across the repo. The only extra match is AbstractLogCollector, which uses it only as a generic type bound (T extends AbstractLogConsumeClient<?, L>) — it does not override initClient0, so no missing override. All 9 real subclasses are in the diff and changed.
  • Master initClient already calls this.close() when isStarted.get() before re-init; that "close previous" path is what makes testFailedReinitializationLeavesClientStopped assert closeCount==1 (init#1 starts, init#2 closes it then fails). Behavior is internally consistent.
  • Existing ClickHouseLogCollectClientTest.testConsume / PulsarLogCollectClientTest.testConsume changed from expecting a thrown exception to assertDoesNotThrow — correct now that a failed init no longer marks the client started.

Conclusion

Correct, well-scoped fix with proper cleanup and good test coverage. Approving.

…rks_started_even_when_initClient0_failed_NPE_in_consume0
@Aias00
Aias00 merged commit 08e92ab into apache:master Aug 23, 2026
41 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] AbstractLogConsumeClient.initClient marks started even when initClient0 bailed — NPE in consume0 (Kafka/RocketMQ/Pulsar/Tencent/RabbitMQ)

2 participants