Skip to content

Fix AsyncHttpEncryptionAwareConduitFactory thread leak on WinRMService close - #85

Merged
bertysentry merged 4 commits into
mainfrom
copilot/fix-async-http-conduit-destruction
Jul 21, 2026
Merged

Fix AsyncHttpEncryptionAwareConduitFactory thread leak on WinRMService close#85
bertysentry merged 4 commits into
mainfrom
copilot/fix-async-http-conduit-destruction

Conversation

Copilot AI commented May 20, 2026

Copy link
Copy Markdown
Contributor

AsyncHttpEncryptionAwareConduitFactory spawns a CloseIdleConnectionThread on construction but its shutdown() is never called, leaving background threads running after WinRMService.close().

Changes

  • WinRMService.close(): Added shutdownConduitFactory() helper that retrieves the AsyncHTTPConduitFactory from the client's endpoint properties (keyed by HTTPConduitFactory.class.getName()) and calls shutdown() on it before client.destroy() — for both cmdClient and wqlClient.

  • WinRMInvocationHandler.getWebServiceClient(): Register the conduit factory on the endpoint only once instead of replacing it on every call. During authentication retries the method is re-invoked on the same client, whose cached conduit keeps using the factory it was created with — replacing the property only orphaned factory instances, and would have left close() shutting down the wrong one. Reusing the factory guarantees close() targets the instance that owns the background threads.

  • WinRMServiceTest: Added testCloseShutdownsConduitFactories verifying that shutdown() is called on the registered factory for both clients when close() reduces the use-count to zero.

// WinRMService.close() — now drains background threads before destroying clients
if (cmdClient != null) {
    shutdownConduitFactory(cmdClient);  // stops CloseIdleConnectionThread
    cmdClient.destroy();
}

private void shutdownConduitFactory(final Client client) {
    final Object factory = client.getEndpoint().getEndpointInfo()
        .getProperty(HTTPConduitFactory.class.getName());
    if (factory instanceof AsyncHTTPConduitFactory) {
        ((AsyncHTTPConduitFactory) factory).shutdown();
    }
}

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • schemas.dmtf.org
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-exports=jdk.xml.dom/org.w3c.dom.html=ALL-UNNAMED --add-exports=java.xml/com.sun.org.apache.xerces.internal.impl.xs=ALL-UNNAMED --add-opens java.base/java.security=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.util.concurrent=ALL-UNNAMED -jar /tmp/cxf-tmp-5752734876370477597/cxf-codegen6026574321731572209.jar /tmp/cxf-tmp-5752734876370477597/cxf-w2j12693372498432677126args (dns block)
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-exports=jdk.xml.dom/org.w3c.dom.html=ALL-UNNAMED --add-exports=java.xml/com.sun.org.apache.xerces.internal.impl.xs=ALL-UNNAMED --add-opens java.base/java.security=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.util.concurrent=ALL-UNNAMED -jar /tmp/cxf-tmp-14258761027604973883/cxf-codegen12271205995426713979.jar /tmp/cxf-tmp-14258761027604973883/cxf-w2j10518240929801386728args (dns block)
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-exports=jdk.xml.dom/org.w3c.dom.html=ALL-UNNAMED --add-exports=java.xml/com.sun.org.apache.xerces.internal.impl.xs=ALL-UNNAMED --add-opens java.base/java.security=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.util.concurrent=ALL-UNNAMED -jar /tmp/cxf-tmp-17700549494455899307/cxf-codegen3872050582524597490.jar /tmp/cxf-tmp-17700549494455899307/cxf-w2j7804011468224100303args (dns block)
  • www.puppycrawl.com
    • Triggering command: /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/bin/java /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/bin/java -jar /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/xml/tools/xml-extractor.jar --fileList=/tmp/codeql-scratch-edd81b6d958bffb3/dbs/java/working/files-to-index7616153095043599934.list --sourceArchiveDir=/tmp/codeql-scratch-edd81b6d958bffb3/dbs/java/src --outputDir=/tmp/codeql-scratch-edd81b6d958bffb3/dbs/java/trap/java (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

… closing WinRMService

Agent-Logs-Url: https://github.com/MetricsHub/winrm-java/sessions/266b5281-d5d0-456c-9f38-84648d52a6c0

Co-authored-by: bertysentry <32521698+bertysentry@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix AsyncHttpEncryptionAwareConduitFactory destruction on WinRMService close Fix AsyncHttpEncryptionAwareConduitFactory thread leak on WinRMService close May 20, 2026
Copilot AI requested a review from bertysentry May 20, 2026 06:17
bertysentry and others added 2 commits July 21, 2026 13:09
- getWebServiceClient: register the AsyncHttpEncryptionAwareConduitFactory
  only once instead of shutting down and replacing it on every call. The
  client's cached conduit keeps using the factory it was created with, so
  replacing the endpoint property only orphaned factory instances (which
  hold no threads until a conduit uses them), and shutting down the
  in-use factory made AsyncHTTPConduit.setupConnection silently fall back
  to the synchronous transport on the retry request. Reusing the factory
  also guarantees WinRMService.close() shuts down the instance that owns
  the background threads.
- WinRMService.shutdownConduitFactory: make it an instance method so the
  static mock of WinRMService in tests cannot swallow it.
- WinRMServiceTest: pass null ticketCache/authentications so the call
  matches the class-level createInstance stub that calls the real method
  (the any(Path.class) matcher does not match null, so the mocked
  createInstance returned null and the test failed with an NPE).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@bertysentry

Copy link
Copy Markdown
Contributor

Took this over to finalize. Review findings and changes (commit 08e8b60):

Kept: the WinRMService.close() fix. The root cause is confirmed: AsyncHttpEncryptionAwareConduitFactory calls the Map-based super constructor of CXF's AsyncHTTPConduitFactory, which — unlike the Bus-based constructor — never registers a BusLifeCycleListener, so bus.shutdown(true) in close() never stopped the factory's CloseableHttpAsyncClient (I/O reactor threads + CXFCloseIdleConnectionThread). Explicitly shutting the factory down on final close is the right fix.

Reworked: the getWebServiceClient() retry change. The original change shut down the existing factory before replacing it. However, the client's conduit selector caches the conduit created from the first factory, so:

  • replacement factories registered on retries are never used to create a conduit and therefore hold no threads — the claimed secondary leak doesn't actually occur;
  • shutting down the in-use factory makes AsyncHTTPConduit.setupConnection (which checks factory.isShutdown()) silently fall back to the synchronous transport on the very retry request that must succeed;
  • after a retry, the endpoint property would point at an unused factory, so the new close() logic would shut down the wrong instance and the original leak would persist.

Instead, the factory is now registered only if absent, so the endpoint property always references the single factory the conduit actually uses, and close() shuts down the right one.

Fixed: the new test. It failed as written for two reasons: createInstance(..., singletonList(NTLM)) matched no class-level stub (any(Path.class) doesn't match null), so the mocked createInstance returned null; and shutdownConduitFactory being private static meant mockStatic(WinRMService.class) swallowed the real method, making the behavior untestable. It's now an instance method and the test passes null ticketCache/authentications.

Also merged main in (JAX-WS catalog + CXF 4.2.1 bumps). mvn verify on JDK 17: 31/31 tests pass, prettier/checkstyle/pmd clean.

@bertysentry
bertysentry marked this pull request as ready for review July 21, 2026 11:23
@bertysentry
bertysentry merged commit e8e1a39 into main Jul 21, 2026
5 checks passed
@bertysentry
bertysentry deleted the copilot/fix-async-http-conduit-destruction branch July 21, 2026 12:37
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.

Destroy AsyncHttpEncryptionAwareConduitFactory object when WinRMService instance is closing

2 participants