Skip to content

Conversation

@tonygermano
Copy link
Member

Commit 1

Make several private fields final in Client

These fields are all either initialized in their declarations or in the
constructor and then never reassigned.

Commit 2

Make Client AutoCloseable and close() idempotent

- Make AutoClosable so it can be used in try-with-resources
- Removed null check for serverConnection since it cannot be null
- Do not shutdown serverConnection or close JAX-RS client if this client
  is already closed.

These fields are all either initialized in their declarations or in the
constructor and then never reassigned.

Signed-off-by: Tony Germano <tony@germano.name>
- Make AutoClosable so it can be used in try-with-resources
- Removed null check for serverConnection since it cannot be null
- Do not shutdown serverConnection or close JAX-RS client if this client
  is already closed.

Signed-off-by: Tony Germano <tony@germano.name>
@tonygermano tonygermano requested review from a team, NicoPiel, jonbartels, kayyagari, kpalang, mgaffigan and pacmano1 and removed request for a team December 24, 2025 02:21
@mgaffigan
Copy link
Contributor

I love boring refactors!

Copy link

@kayyagari kayyagari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, "I reviewed it' ;).

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request enhances the Client class by making it AutoCloseable and improving immutability through making several fields final. The changes enable the Client to be used with try-with-resources statements while ensuring thread-safe, idempotent resource cleanup.

Key changes:

  • Made five private fields (logger, serverConnection, client, api, closed) final to prevent reassignment
  • Implemented the AutoCloseable interface to support try-with-resources usage
  • Enhanced the close() method to be idempotent and thread-safe using atomic operations
Comments suppressed due to low confidence (1)

server/src/com/mirth/connect/client/core/Client.java:325

  • The new AutoCloseable functionality and idempotent close() implementation lack test coverage. Consider adding tests to verify:
  1. Multiple calls to close() are safe and only shutdown resources once
  2. Client can be used with try-with-resources
  3. Calling close() properly shuts down both serverConnection and the JAX-RS client
    @Override
    public void close() {
        if (!closed.getAndSet(true)) {
            serverConnection.shutdown();
            client.close();
        }
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@jonbartels jonbartels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM #modernJava

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.

5 participants