Skip to content

Commit

Permalink
GH-2471: MQTT: Fix Thread Leak
Browse files Browse the repository at this point in the history
Fixes #2471

Call `close()` on the client whenever the connection is lost or can't be
established, to release resources in the client.

**cherry-pick to 5.0.x, 4.3.x**

(cherry picked from commit f9cea64)

# Conflicts:
#	spring-integration-mqtt/src/test/java/org/springframework/integration/mqtt/MqttAdapterTests.java
  • Loading branch information
garyrussell authored and artembilan committed Jun 6, 2018
1 parent 37c92a1 commit 81cfa80
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 1 deletion.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2017 the original author or authors.
* Copyright 2002-2018 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 @@ -262,6 +262,14 @@ private synchronized void connectAndSubscribe() throws MqttException {
}
logger.error("Error connecting or subscribing to " + Arrays.toString(topics), e);
this.client.disconnectForcibly(this.completionTimeout);
try {
this.client.setCallback(null);
this.client.close();
}
catch (MqttException e1) {
// NOSONAR
}
this.client = null;
throw e;
}
finally {
Expand Down Expand Up @@ -321,6 +329,14 @@ public synchronized void connectionLost(Throwable cause) {
if (isRunning()) {
this.logger.error("Lost connection: " + cause.getMessage() + "; retrying...");
this.connected = false;
try {
this.client.setCallback(null);
this.client.close();
}
catch (MqttException e) {
// NOSONAR
}
this.client = null;
scheduleReconnect();
if (this.applicationEventPublisher != null) {
this.applicationEventPublisher.publishEvent(new MqttConnectionFailedEvent(this, cause));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import static org.mockito.BDDMockito.given;
import static org.mockito.BDDMockito.willAnswer;
import static org.mockito.BDDMockito.willReturn;
import static org.mockito.BDDMockito.willThrow;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyLong;
import static org.mockito.Matchers.anyString;
Expand Down Expand Up @@ -98,6 +99,16 @@ public class MqttAdapterTests {
this.alwaysComplete = (IMqttToken) pfb.getObject();
}

@Test
public void testCloseOnBadConnect() throws Exception {
final IMqttClient client = mock(IMqttClient.class);
willThrow(new MqttException(0)).given(client).connect(any());
MqttPahoMessageDrivenChannelAdapter adapter = buildAdapter(client, null, ConsumerStopAction.UNSUBSCRIBE_NEVER);
adapter.start();
verify(client).close();
adapter.stop();
}

@Test
public void testOutboundOptionsApplied() throws Exception {
DefaultMqttPahoClientFactory factory = new DefaultMqttPahoClientFactory();
Expand Down Expand Up @@ -345,6 +356,7 @@ public void testReconnect() throws Exception {
adapter.setTaskScheduler(taskScheduler);
adapter.start();
adapter.connectionLost(new RuntimeException("initial"));
verify(client).close();
Thread.sleep(1000);
// the following assertion should be equalTo, but leq to protect against a slow CI server
assertThat(attemptingReconnectCount.get(), lessThanOrEqualTo(2));
Expand Down

0 comments on commit 81cfa80

Please sign in to comment.