-
Notifications
You must be signed in to change notification settings - Fork 41
AutoClosable Client #232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
AutoClosable Client #232
Conversation
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>
|
I love boring refactors! |
kayyagari
left a comment
There was a problem hiding this 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' ;).
There was a problem hiding this 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:
- Multiple calls to close() are safe and only shutdown resources once
- Client can be used with try-with-resources
- 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.
jonbartels
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM #modernJava
Commit 1
Commit 2