Make Channel and Session implement AutoClosable#1052
Conversation
|
norrisjeremy
left a comment
norrisjeremy
left a comment
There was a problem hiding this comment.
As outlined in my comments, changing the Session & Channel classes to implement AutoCloseable isn't correct.
| */ | ||
|
|
||
| void close() { | ||
| public void close() { |
There was a problem hiding this comment.
This isn't correct: the close() method here isn't meant for public consumption, as it does not fully perform all necessary cleanup: it is meant as an internal-use only method.
The correct way to dispose of a Channel object is to call disconnect().
There was a problem hiding this comment.
Also, I believe Channel may have similar semantics as Session, in that it may be re-usable after calling disconnect(), which disagrees with the semantics of AutoCloseable.
| } | ||
|
|
||
| public void close() { | ||
| this.disconnect(); |
There was a problem hiding this comment.
I don't think introducing AutoCloseable semantics to a Session object is correct: there is an expectation that Session objects are re-usable after users call the disconnect() method, which doesn't agree with the semantics of AutoCloseable, which implys that after an object is fully disposed after calling the close() method.
|
I closed this PR. As I found this repo only through an PR of an co-worker. My PR was based on the assumption that a channel should be "closed" when disconnecting. |
2 participants
I would argue that both Channel and Session should implement AutoCloseable to enable try-with-resources usage.
However, I'm not entirely sure if my suggested implementation is correct—especially for Channel. I can see a scenario where disconnect() should be the function called during the implicit close.