Both setup paths go through here so that they cannot drift apart. They previously had separate copies of + * this logic, and the copy in setupEccDevice was missing the unauthenticated branch entirely.
+ * + * @param optionsBuilder The builder to apply the proxy settings to + */ + private void applyProxySettings(ClientOptions.ClientOptionsBuilder optionsBuilder) { - ClientOptions.ClientOptionsBuilder optionsBuilder = ClientOptions.builder(); - if (this.useHttpProxy) + if (!this.useHttpProxy) { - if (this.useHttpProxyAuth) - { - Proxy testProxy = new Proxy(Proxy.Type.HTTP, new InetSocketAddress(testProxyHostname, testProxyPort)); - optionsBuilder.proxySettings(new ProxySettings(testProxy, testProxyUser, testProxyPass.toCharArray())); - } - else - { - Proxy testProxy = new Proxy(Proxy.Type.HTTP, new InetSocketAddress(testProxyHostnameWithoutAuth, testProxyPortWithoutAuth)); - optionsBuilder.proxySettings(new ProxySettings(testProxy)); - } + return; + } + + if (this.useHttpProxyAuth) + { + Proxy testProxy = new Proxy(Proxy.Type.HTTP, new InetSocketAddress(testProxyHostname, testProxyPort)); + optionsBuilder.proxySettings(new ProxySettings(testProxy, testProxyUser, testProxyPass.toCharArray())); + } + else + { + Proxy testProxy = new Proxy(Proxy.Type.HTTP, new InetSocketAddress(testProxyHostnameWithoutAuth, testProxyPortWithoutAuth)); + optionsBuilder.proxySettings(new ProxySettings(testProxy)); } + } + + public void setup() throws Exception + { + ClientOptions.ClientOptionsBuilder optionsBuilder = ClientOptions.builder(); + applyProxySettings(optionsBuilder); if (clientType == ClientType.DEVICE_CLIENT) { - this.identity = Tools.getTestDevice(iotHubConnectionString, this.protocol, this.authenticationType, false, optionsBuilder); + trackForCleanup(Tools.getTestDevice(iotHubConnectionString, this.protocol, this.authenticationType, false, optionsBuilder)); } else if (clientType == ClientType.MODULE_CLIENT) { - this.identity = Tools.getTestModule(iotHubConnectionString, this.protocol, this.authenticationType , false, optionsBuilder); + trackForCleanup(Tools.getTestModule(iotHubConnectionString, this.protocol, this.authenticationType , false, optionsBuilder)); } + + disposeIfTeardownAlreadyRan(); } public void setupEccDevice() throws Exception { ClientOptions.ClientOptionsBuilder optionsBuilder = ClientOptions.builder(); - if (this.useHttpProxy) - { - Proxy testProxy = new Proxy(Proxy.Type.HTTP, new InetSocketAddress(testProxyHostname, testProxyPort)); - optionsBuilder.proxySettings(new ProxySettings(testProxy, testProxyUser, testProxyPass.toCharArray())); - } + applyProxySettings(optionsBuilder); X509CertificateGenerator certificateGenerator = new X509CertificateGenerator(X509CertificateGenerator.CertificateAlgorithm.ECC); SSLContext sslContext = SSLContextBuilder.buildSSLContext(certificateGenerator.getX509Certificate(), certificateGenerator.getPrivateKey()); @@ -157,11 +190,12 @@ public void setupEccDevice() throws Exception eccDevice.setThumbprint(certificateGenerator.getX509Thumbprint(), certificateGenerator.getX509Thumbprint()); Tools.addDeviceWithRetry(new RegistryClient(iotHubConnectionString), eccDevice); + trackEccDeviceForCleanup(eccDevice.getDeviceId()); String deviceConnectionString = Tools.getDeviceConnectionString(iotHubConnectionString, eccDevice); - this.identity = new TestDeviceIdentity( + trackForCleanup(new TestDeviceIdentity( new DeviceClient(deviceConnectionString, testInstance.protocol, optionsBuilder.build()), - eccDevice); + eccDevice)); } else if (clientType == ClientType.MODULE_CLIENT) { @@ -171,24 +205,122 @@ else if (clientType == ClientType.MODULE_CLIENT) eccModule.setThumbprint(certificateGenerator.getX509Thumbprint(), certificateGenerator.getX509Thumbprint()); Tools.addDeviceWithRetry(new RegistryClient(iotHubConnectionString), eccDevice); + trackEccDeviceForCleanup(eccDevice.getDeviceId()); + Tools.addModuleWithRetry(new RegistryClient(iotHubConnectionString), eccModule); String moduleConnectionString = Tools.getDeviceConnectionString(iotHubConnectionString, eccDevice) + ";ModuleId=" + eccModule.getId(); - this.identity = new TestModuleIdentity( + trackForCleanup(new TestModuleIdentity( new ModuleClient(moduleConnectionString, testInstance.protocol, optionsBuilder.build()), eccDevice, - eccModule); + eccModule)); + } + + disposeIfTeardownAlreadyRan(); + } + + /** + * Hand an identity to teardown, and publish it for the test body to use. + * + * @param newIdentity The identity this test just acquired or created + */ + private void trackForCleanup(TestIdentity newIdentity) + { + // Published for the test body. Never cleared, so a thread the JUnit timeout abandoned can keep reading it. + this.identity = newIdentity; + + synchronized (lifecycleLock) + { + this.identityToDispose = newIdentity; } } + /** + * Hand a freshly registered ECC device to teardown, so it is removed from the registry even if the rest of + * setupEccDevice never completes. + * + * @param deviceId The device id that was just added to the registry + */ + private void trackEccDeviceForCleanup(String deviceId) + { + synchronized (lifecycleLock) + { + this.eccDeviceIdToDelete = deviceId; + } + } + + /** + * Dispose anything registered after teardown already ran. + * + *Every test in this class is bounded by {@code @Test(timeout = 60000)}. JUnit runs the test body on a + * separate thread and, when the timeout fires, abandons that thread while it is still running. {@code @After} + * is outside the timeout, so teardown can execute while setup on the abandoned thread has not finished + * acquiring its identity. Without this, the identity that setup goes on to produce would have no owner and + * would leak, which is precisely the leak this class is trying to stop.
+ * + *This does not wait for setup, in either direction. Blocking teardown on a setup that is itself hung - + * which is how these tests have actually timed out - would stall the rest of the run.
+ */ + private void disposeIfTeardownAlreadyRan() + { + boolean teardownAlreadyRan; + synchronized (lifecycleLock) + { + teardownAlreadyRan = this.disposed; + } + + if (teardownAlreadyRan) + { + dispose(); + } + } + + /** + * Close and dispose whatever this instance currently owns. + * + *Safe to call more than once, and safe to call concurrently with setup: it takes ownership of the tracked + * fields and clears them, so a second call finds only what was registered since the first.
+ */ public void dispose() { - if (this.identity != null && this.identity.getClient() != null) + TestIdentity identityToClean; + String eccDeviceIdToClean; + + synchronized (lifecycleLock) + { + this.disposed = true; + + identityToClean = this.identityToDispose; + eccDeviceIdToClean = this.eccDeviceIdToDelete; + + this.identityToDispose = null; + this.eccDeviceIdToDelete = null; + } + + if (identityToClean != null && identityToClean.getClient() != null) { - this.identity.getClient().close(); + identityToClean.getClient().close(); } - Tools.disposeTestIdentity(this.identity, iotHubConnectionString); + if (eccDeviceIdToClean != null) + { + // Recycling this identity would hand a device carrying a certificate that no other test knows about to + // the next test that takes an x509 identity from the shared pool, so delete it instead. This runs even + // when the identity was never finished being built, because the device is in the registry from the + // moment it is registered, whether or not the rest of the setup succeeded. + try + { + Tools.getRegistyManager(iotHubConnectionString).removeDevice(eccDeviceIdToClean); + } + catch (IOException | IotHubException e) + { + log.error("Failed to clean up ECC test device {}", eccDeviceIdToClean, e); + } + } + else if (identityToClean != null) + { + Tools.disposeTestIdentity(identityToClean, iotHubConnectionString); + } } } @@ -208,18 +340,28 @@ public void dispose() protected static final String testProxyPass = "1234"; // lgtm @BeforeClass - public static void startProxy() + public static void startProxy() throws Exception { HttpProxyServerConfig config = new HttpProxyServerConfig(); config.setAuthenticationProvider(new BasicProxyAuthenticator(testProxyUser, testProxyPass)); config.setHandleSsl(false); proxyServer = new HttpProxyServer().serverConfig(config); - proxyServer.startAsync(testProxyPort); + ProxyServerTools.startProxyServer(proxyServer, testProxyPort); HttpProxyServerConfig configWithoutAuth = new HttpProxyServerConfig(); configWithoutAuth.setHandleSsl(false); proxyServerWithoutAuth = new HttpProxyServer().serverConfig(configWithoutAuth); - proxyServerWithoutAuth.startAsync(testProxyPortWithoutAuth); + ProxyServerTools.startProxyServer(proxyServerWithoutAuth, testProxyPortWithoutAuth); + } + + // Without this, every test in this class leaks the client it opened. Those clients keep retrying their + // connections for the rest of the JVM's life, and the ones configured with proxy settings keep retrying through + // the proxies this class runs locally, which competes with the tests that are still running. Once stopProxy has + // closed those proxies they retry against a dead port instead, for the remainder of the job. + @After + public void disposeTestInstance() + { + testInstance.dispose(); } @AfterClass @@ -273,16 +415,21 @@ private static void logConnectionStatusChanges(InternalClient client) public void CanOpenConnection() throws Exception { testInstance.setup(); - logConnectionStatusChanges(testInstance.identity.getClient()); - testInstance.identity.getClient().open(true); + + // Held locally rather than read back off testInstance for each call. The @After that disposes this instance + // runs outside this method's timeout, so it can execute while this thread is still here after a timeout. + InternalClient client = testInstance.identity.getClient(); + + logConnectionStatusChanges(client); + client.open(true); // deviceClient.open() is a no-op on HTTP, so a message needs to be sent to actually test opening the connection if (testInstance.protocol == HTTPS) { - testInstance.identity.getClient().sendEvent(new Message("some message")); + client.sendEvent(new Message("some message")); } - testInstance.identity.getClient().close(); + client.close(); } @IotHubTest @@ -303,16 +450,18 @@ public void CanOpenConnectionWithECCCertificates() throws Exception testInstance.setupEccDevice(); - logConnectionStatusChanges(testInstance.identity.getClient()); - testInstance.identity.getClient().open(true); + InternalClient client = testInstance.identity.getClient(); + + logConnectionStatusChanges(client); + client.open(true); // deviceClient.open() is a no-op on HTTP, so a message needs to be sent to actually test opening the connection if (testInstance.protocol == HTTPS) { - testInstance.identity.getClient().sendEvent(new Message("some message")); + client.sendEvent(new Message("some message")); } - testInstance.identity.getClient().close(); + client.close(); } @Test(timeout = 60000) // 1 minute @@ -360,10 +509,22 @@ public void CanOpenMultiplexingConnection() throws Exception multiplexingClient.registerDeviceClients(testClients); multiplexingClient.open(true); - multiplexingClient.close(); } finally { + // Closing here rather than after open() so that a failed or timed out open still gives the client and the + // three device clients registered to it back. Otherwise they keep retrying for the life of the JVM, and + // the proxied variants keep retrying through the proxies this class runs locally. + try + { + multiplexingClient.close(); + } + catch (Exception e) + { + // Swallowed so it cannot mask whatever the test itself threw. + log.error("Failed to close the multiplexing client", e); + } + Tools.disposeTestIdentities(testIdentities, iotHubConnectionString); } } diff --git a/provisioning/provisioning-device-client/src/test/java/com/microsoft/azure/sdk/iot/provisioning/device/internal/contract/amqp/ContractAPIAmqpTest.java b/provisioning/provisioning-device-client/src/test/java/com/microsoft/azure/sdk/iot/provisioning/device/internal/contract/amqp/ContractAPIAmqpTest.java index ef4f44bc06..b02cd55070 100644 --- a/provisioning/provisioning-device-client/src/test/java/com/microsoft/azure/sdk/iot/provisioning/device/internal/contract/amqp/ContractAPIAmqpTest.java +++ b/provisioning/provisioning-device-client/src/test/java/com/microsoft/azure/sdk/iot/provisioning/device/internal/contract/amqp/ContractAPIAmqpTest.java @@ -67,9 +67,6 @@ public class ContractAPIAmqpTest @Mocked Map