Skip to content

Commit

Permalink
GH-2744: Fix Possible Deadlock in DKPF
Browse files Browse the repository at this point in the history
Resolves #2744

Possible deadlock if `removeProducer` is called on the producer network thread.

Move resetting the global shared producer to the creation logic.

Also ensure the delegate of any thread-bound producers are closed.

Add try/catch around the delegate close.

**cherry-pick to 2.9.x**
# Conflicts:
#	spring-kafka/src/test/java/org/springframework/kafka/core/DefaultKafkaProducerFactoryTests.java
  • Loading branch information
garyrussell authored and artembilan committed Jul 17, 2023
1 parent 8404ef5 commit bd57099
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 19 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2022 the original author or authors.
* Copyright 2016-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -751,6 +751,10 @@ private Producer<K, V> doCreateProducer(@Nullable String txIdPrefix) {
return getOrCreateThreadBoundProducer();
}
synchronized (this) {
if (this.producer != null && this.producer.closed) {
this.producer.closeDelegate(this.physicalCloseTimeout, this.listeners);
this.producer = null;
}
if (this.producer != null && expire(this.producer)) {
this.producer = null;
}
Expand All @@ -765,7 +769,7 @@ private Producer<K, V> doCreateProducer(@Nullable String txIdPrefix) {

private Producer<K, V> getOrCreateThreadBoundProducer() {
CloseSafeProducer<K, V> tlProducer = this.threadBoundProducers.get();
if (tlProducer != null && (this.epoch.get() != tlProducer.epoch || expire(tlProducer))) {
if (tlProducer != null && (tlProducer.closed || this.epoch.get() != tlProducer.epoch || expire(tlProducer))) {
closeThreadBoundProducer();
tlProducer = null;
}
Expand Down Expand Up @@ -834,21 +838,11 @@ private boolean removeConsumerProducer(CloseSafeProducer<K, V> producerToRemove,
* Remove the single shared producer and a thread-bound instance if present.
* @param producerToRemove the producer.
* @param timeout the close timeout.
* @return always true.
* @return true if the producer was closed.
* @since 2.2.13
*/
protected final synchronized boolean removeProducer(CloseSafeProducer<K, V> producerToRemove, Duration timeout) {
if (producerToRemove.closed) {
if (producerToRemove.equals(this.producer)) {
this.producer = null;
producerToRemove.closeDelegate(timeout, this.listeners);
}
this.threadBoundProducers.remove();
return true;
}
else {
return false;
}
protected final boolean removeProducer(CloseSafeProducer<K, V> producerToRemove, Duration timeout) {
return producerToRemove.closed;
}

/**
Expand Down Expand Up @@ -1204,7 +1198,12 @@ public void close(@Nullable Duration timeout) {
}

void closeDelegate(Duration timeout, List<Listener<K, V>> listeners) {
this.delegate.close(timeout == null ? this.closeTimeout : timeout);
try {
this.delegate.close(timeout == null ? this.closeTimeout : timeout);
}
catch (Exception ex) {
LOGGER.warn(ex, () -> "Failed to close " + this.delegate);
}
listeners.forEach(listener -> listener.producerRemoved(this.clientId, this));
this.closed = true;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2018-2021 the original author or authors.
* Copyright 2018-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -331,11 +331,12 @@ protected Producer createKafkaProducer() {
};
final Producer aProducer = pf.createProducer();
assertThat(aProducer).isNotNull();
Producer bProducer = pf.createProducer();
assertThat(bProducer).isSameAs(aProducer);
aProducer.send(null, (meta, ex) -> { });
aProducer.close(ProducerFactoryUtils.DEFAULT_CLOSE_TIMEOUT);
assertThat(KafkaTestUtils.getPropertyValue(pf, "producer")).isNull();
bProducer = pf.createProducer();
verify(producer1).close(any(Duration.class));
Producer bProducer = pf.createProducer();
assertThat(bProducer).isNotSameAs(aProducer);
}

Expand Down

0 comments on commit bd57099

Please sign in to comment.