-
Notifications
You must be signed in to change notification settings - Fork 585
Channel should not implement Autoclosable because a channel is expected to stay open. #773
-
Channel should not implement Autoclosable because a channel is expected to stay open for the duration of the application, if not longer.
I get a SonarLint error for the following code:
Channel channel = connection.createChannel();
==============================================
Resources should be closed
Bug
Blocker
java:S2095
Connections, streams, files, and other classes that implement the Closeable interface or its super-interface, AutoCloseable, needs to be closed after use. Further, that close call must be made in a finally block otherwise an exception could keep the call from being made. Preferably, when class implements AutoCloseable, resource should be created using "try-with-resources" pattern and will be closed automatically.
Failure to properly close resources will result in a resource leak which could bring first the application and then perhaps the box the application is on to their knees.
Beta Was this translation helpful? Give feedback.
All reactions
Replies: 4 comments 2 replies
-
I don't think we should do that. All these static code analysers by definition are subjective
in what they flag as "bugs". Some codebases, for better or worse, use try-with-resources with channels or use short lived channels. We cannot claim that it is universally wrong.
Nothing in AutoClosable docs suggests that a resource that stays open for a long period of time cannot implement it.
Please add an exclusion (I'm sure Sonar allows for that) and move on.
Beta Was this translation helpful? Give feedback.
All reactions
-
"Move on."
In what use case would Autoclosable be useful for a Channel?
GFY
Beta Was this translation helpful? Give feedback.
All reactions
-
try-with-resources is primarily used in tutorials and for one-off channels (passive declares and such) if I had to guess. AutoCloseable was introduced
in #258 over five years ago and I don't remember a single other suggestion that it is somehow harmful or semantically wrong.
FYI, GFY has more than one meaning.
I don't assume that people on the Internet want to intentionally offend OSS maintainers but
this one is easy to misinterpret.
Beta Was this translation helpful? Give feedback.
All reactions
-
In what use case would Autoclosable be useful for a Channel?
An application that very rarely publishes messages may wish to use a try-with-resources block to ensure the channel is closed after publishing a message. There's nothing inherently "wrong" with a connection/channel per-message pattern if you know what you're doing.
Beta Was this translation helpful? Give feedback.
All reactions
-
To wrap this up: we will keep AutoCloseable because channels do not really conflict with the documented semantics of AutoCloseable, at least until we see a lot more suggestions to drop it.
AutoCloseable is used in connection management classes in other libraries, including some
maintained by Oracle engineers.
We will also treat the above GFY comment by @karlnicholas as an intentional insult and apply adequate restrictions to his account for the entire RabbitMQ org.
Beta Was this translation helpful? Give feedback.
All reactions
-
Channel should not implement Autoclosable because a channel is expected to stay open for the duration of the application, if not longer.
The definition of AutoCloseable does not mention the duration of the resource, it must be closed at some point, that's all.
In what use case would Autoclosable be useful for a Channel?
Being able to use the try-with-resources statement and notifying developers that instances of this class take up resources and must be closed at some point (what Sonar did).
AutoCloseable is used all over the places in the JDK (JDBC, IO, etc) and in Jakarta EE (JMS 2.0 adopted it when it was released).
Item 9 of Effective Java 3rd edition covers the benefits of try-with-resources and AutoCloseable and I'd say RabbitMQ Java Client's Connection and Channel fit pretty well the cases where AutoCloseable is appropriate.
I stand by my colleague and suggest you use the //NOSONAR mechanism to suppress the issue.
Beta Was this translation helpful? Give feedback.