KAFKA-10790: Add deadlock detection to producer#flush by frankvicky · Pull Request #17946 · apache/kafka

frankvicky · 2024-11-26T14:43:41Z

As we already know, the callback of Producer#send is executed by the ioThread. Due to this design, many application bugs have been caused by calling Producer#flush in callback，which leads to deadlocks. This PR aims to add protection against such scenarios.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

AndrewJSchofield

Thanks for the PR. This seems like a useful enhancement to me.

Please update the upgrade documentation too. This is a small change in behaviour which will affect anyone who is using flush() inside a callback.

AndrewJSchofield · 2024-11-26T17:29:57Z

+    public void shouldNotInvokeFlushInCallback() {
+        Map<String, Object> configs = new HashMap<>();
+        configs.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9000");
+        configs.put(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, false);


Is this setting relevant? Having it in this test makes me think that the test would only work with it, but I do not believe that to be the case.

This setting aims to simplify our test case. Here's my understanding—please correct me if I'm wrong:
Since idempotence is enabled by default, if we don't explicitly set it to false, we would need to prepare additional data to go through the idempotence flow. I believe this is unnecessary for this test case, so I will add a comment to clarify. WDYT?

OK, I see. It prevents needing to do the init producer id stuff. Makes sense to me.

AndrewJSchofield · 2024-11-26T17:35:37Z

    @Override
    public void flush() {
+        if (Thread.currentThread() == this.ioThread) {
+            log.error("flush invocation detected in the callback. This can cause a deadlock due to thread blocking.");


I would change the wording here because actually the code is preventing the call and that means there is no risk of a deadlock. Something like "KafkaProducer.flush() invocation detected inside callback. This is not permitted because of the risk of deadlock."

The message in the exception could be the same. The choice of KafkaException seems most appropriate to me.

I would like to know this log is necessary? In my opinion, this information is already recorded by KafkaException.

I think it's ok to use both logging and exceptions simultaneously, as the user might want immediate hints displayed on the console.

TaiJuWu

LGTM

AndrewJSchofield

I suggest that the KafkaProducer javadoc should be altered as well. That's the most likely place that a programmer will notice.

AndrewJSchofield · 2024-11-28T21:19:53Z

                        </li>
                        <li>The deprecated <code>sendOffsetsToTransaction(Map&lt;TopicPartition, OffsetAndMetadata&gt;, String)</code> method has been removed from the Producer API.
                        </li>
+                        <li>The <code>flush</code> method now includes deadlock detection when invoked inside a callback to prevent unintended blocking scenarios.


This could be more explicit, like the message in the exception added to KafkaProducer. You are not allowed to use flush() in the callback. Maybe "The flush method now includes deadlock detection preventing its use inside a callback. This avoids unintended blocking which has been known to occur prior to this change." Something like that. We want someone reading this to understand that they must not use flush in the callback.

AndrewJSchofield · 2024-11-29T09:16:06Z

The code looks good to me, but I don't think we are ready to merge yet.

I've been thinking about the mechanics of delivering this into AK, and this is definitely a change in behaviour of a public interface, which means a KIP is required. It's true that it's preventing an unsafe behaviour which is categorically a good thing, but if someone has an application which is doing this, their code will break following this change. @frankvicky, do you agree? Can you create a KIP to make sure we have community alignment?

Without a KIP, we cannot throw the exception, but we can log the message and update the javadoc.

chia7712 · 2024-11-29T09:26:33Z

but if someone has an application which is doing this, their code will break following this change. @frankvicky, do you agree? Can you create a KIP to make sure we have community alignment?
Without a KIP, we cannot throw the exception, but we can log the message and update the javadoc.

Yes, it’s definitely worth having a KIP. The KIP should not only cover the behavior changes but also facilitate discussions on other potential approaches—for instance, adding a log message instead of throwing an exception.

frankvicky · 2024-11-29T09:27:14Z

Hello @AndrewJSchofield
I think you are right. Since Kafka usually considers exceptions as part of the public API, this change will require a KIP.
I will prepare one soon.

frankvicky · 2024-12-02T09:13:17Z

Hello everyone,
I have filed KIP-1118 for this issue, PTAL.

AndrewJSchofield · 2024-12-09T15:44:09Z

Hi @frankvicky. I think we can get some of the benefit here in spite of the fact that the KIP is too late for AK 4.0. I suggest you change this PR to alter that javadoc to say "This method should not be used" rather than "must not". You can log the error if flush is used in the callback. You cannot throw the KafkaException. What do you think?

chia7712 · 2024-12-09T16:22:57Z

I think we can get some of the benefit here in spite of the fact that the KIP is too late for AK 4.0. I suggest you change this PR to alter that javadoc to say "This method should not be used" rather than "must not". You can log the error if flush is used in the callback. You cannot throw the KafkaException. What do you think?

If we want to avoid introducing behavior changes in version 4.0, adding documentation and logging is an appropriate solution. @frankvicky, could you please file a MINOR issue to address @AndrewJSchofield's comment? In the minor PR, we don't introduce any behavior changes but enrich the documentation and logging.

frankvicky · 2024-12-10T00:15:59Z

Hi @AndrewJSchofield @chia7712
No problem. I will file a PR soon.

frankvicky · 2024-12-10T00:39:05Z

This is minor PR: #18112

frankvicky · 2025-01-07T13:08:53Z

Hi @AndrewJSchofield @chia7712
Since the KIP has been accepted, I think we could merge this one.
WDYT?

AndrewJSchofield · 2025-01-07T13:20:09Z

@frankvicky It is unfortunate this behaviour change didn't make the 4.0 deadline. Personally, because the old behaviour was liable to cause deadlocks, I'm good with making such a change in 4.1. @chia7712 What do you think?

AndrewJSchofield

The code looks good to me. Just one documentation comment.

AndrewJSchofield · 2025-01-07T13:21:43Z

                        </li>
                        <li>The deprecated <code>sendOffsetsToTransaction(Map&lt;TopicPartition, OffsetAndMetadata&gt;, String)</code> method has been removed from the Producer API.
                        </li>
+                        <li>The <code>flush</code> method now detects potential deadlocks and prohibits its use inside a callback. This change prevents unintended blocking behavior, which was a known risk in earlier versions.


This text now needs to be in an "upgrading to 4.1" section.

I agree with @AndrewJSchofield that this PR will not be backported to the 4.0 release. Therefore, this change should not be included in the "upgrade_4_0_0" chapter. @frankvicky, could you please create a new chapter titled "upgrade_4_1_0" and move this documentation to the "upgrade_4_1_0" chapter?

I have applied the suggestion. Here is the preview:

…og-compaction-write-record-v2 * apache-github/trunk: (34 commits) MINOR: Bump year to 2025 in NOTICE file (apache#18427) KAFKA-18411 Remove ZkProducerIdManager (apache#18413) KAFKA-18408 tweak the 'tag' field for BrokerHeartbeatRequest.json, BrokerRegistrationChangeRecord.json and RegisterBrokerRecord.json (apache#18421) KAFKA-18414 Remove KRaftRegistrationResult (apache#18401) KAFKA-17921 Support SASL_PLAINTEXT protocol with java.security.auth.login.config (apache#17671) KAFKA-18384 Remove ZkAlterPartitionManager (apache#18364) KAFKA-10790: Add deadlock detection to producer#flush (apache#17946) KAFKA-18412: Remove EmbeddedZookeeper (apache#18399) MINOR : Improve Exception log in NotEnoughReplicasException(apache#12394) MINOR: Improve PlaintextAdminIntegrationTest#testConsumerGroups (apache#18409) MINOR: Remove unused local variable (apache#18410) MINOR: Remove RaftManager.maybeDeleteMetadataLogDir and AutoTopicCreationManagerTest.scala (apache#17365) KAFKA-18368 Remove TestUtils#MockZkConnect and remove zkConnect from TestUtils#createBrokerConfig (apache#18352) MINOR: Update Consumer group timeout default to 30 sec (apache#16406) MINOR: Fix typo in CommitRequestManager (apache#18407) MINOR: cleanup JavaDocs for deprecation warnings (apache#18402) KAFKA-18303; Update ShareCoordinator to use new record format (apache#18396) MINOR: Update Consumer and Producer JavaDocs for committing offsets (apache#18336) KAFKA-16446: Improve controller event duration logging (apache#15622) KAFKA-18388 test-kraft-server-start.sh should use log4j2.yaml (apache#18370) ...

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Andrew Schofield <aschofield@confluent.io>, TaiJuWu <tjwu1217@gmail.com>

github-actions Bot added producer clients small Small PRs labels Nov 26, 2024

FrankYang0529 added the ci-approved label Nov 26, 2024

frankvicky force-pushed the KAFKA-10790 branch 2 times, most recently from e4e2d89 to f595416 Compare November 26, 2024 14:46

AndrewJSchofield requested changes Nov 26, 2024

View reviewed changes

frankvicky force-pushed the KAFKA-10790 branch 2 times, most recently from 2ac6d6b to 0815cef Compare November 27, 2024 09:06

TaiJuWu approved these changes Nov 27, 2024

View reviewed changes

AndrewJSchofield requested changes Nov 28, 2024

View reviewed changes

frankvicky force-pushed the KAFKA-10790 branch from 0815cef to 57a8391 Compare November 29, 2024 02:58

AndrewJSchofield added the kip Requires or implements a KIP label Nov 29, 2024

frankvicky force-pushed the KAFKA-10790 branch 2 times, most recently from 9268221 to abd29bb Compare December 4, 2024 09:10

frankvicky mentioned this pull request Dec 10, 2024

MINOR: Logs warning message when user invoke producer#flush within callback #18112

Merged

3 tasks

frankvicky force-pushed the KAFKA-10790 branch from abd29bb to dd0a77d Compare December 10, 2024 09:26

frankvicky force-pushed the KAFKA-10790 branch 3 times, most recently from 0c3d575 to ae3c967 Compare January 5, 2025 05:10

AndrewJSchofield requested changes Jan 7, 2025

View reviewed changes

frankvicky force-pushed the KAFKA-10790 branch from ae3c967 to 77a0123 Compare January 7, 2025 13:41

KAFKA-10790: Add deadlock detection to producer#flush

81a8946

frankvicky force-pushed the KAFKA-10790 branch from 77a0123 to 81a8946 Compare January 7, 2025 13:42

AndrewJSchofield approved these changes Jan 7, 2025

View reviewed changes

AndrewJSchofield merged commit abeed20 into apache:trunk Jan 7, 2025

pranavt84 pushed a commit to pranavt84/kafka that referenced this pull request Jan 27, 2025

KAFKA-10790: Add deadlock detection to producer#flush (apache#17946)

9bbf95c

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Andrew Schofield <aschofield@confluent.io>, TaiJuWu <tjwu1217@gmail.com>

Conversation

frankvicky commented Nov 26, 2024

Committer Checklist (excluded from commit message)

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TaiJuWu Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TaiJuWu left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewJSchofield commented Nov 29, 2024

Uh oh!

chia7712 commented Nov 29, 2024

Uh oh!

frankvicky commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankvicky commented Dec 2, 2024

Uh oh!

AndrewJSchofield commented Dec 9, 2024

Uh oh!

chia7712 commented Dec 9, 2024

Uh oh!

frankvicky commented Dec 10, 2024

Uh oh!

frankvicky commented Dec 10, 2024

Uh oh!

frankvicky commented Jan 7, 2025

Uh oh!

AndrewJSchofield commented Jan 7, 2025

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TaiJuWu Nov 27, 2024 •

edited

Loading

frankvicky commented Nov 29, 2024 •

edited

Loading