-
Notifications
You must be signed in to change notification settings - Fork 610
Description
I am wanting to cancel an SSHJ connection by issuing a Thread.interrupt.
This has inconsistent behaviour. One of four things happens:
(a) InterruptedException is declared and thrown by the method (e.g. Connection.join()), and the thread is in an interrupted state
(b) Another exception is thrown to me (eg SSHException), wrapping the InterruptedException as its cause, but the thread is not in an interrupted state
(c) Another exception is thrown internally (eg IOException, e.g. by StreamCopier.chainer) which wraps the InterruptedException as its cause, but it is caught and causes a different error, throwing to me another exception which does not wrap the InterruptedException, and the thread is not in an interrupted state
(d) An exception is thrown internally (eg IOException) which wraps the InterruptedException as its cause, but it is caught and either ignored (e.g. in some of the close methods) or retried, and the SSH command continues and might even succeed, returning to me something that looks like success, and the thread is not in an interrupted state
The behaviour (a) is fine, but the others are problematic because I don't have the indications or behaviour I would normally expect that there was an interrupt (the thread not on an interrupted state). The biggest offender is the StreamCopier which returns an `
For me, the ideal behaviour (whenever (a) is unattractive on an API, which is a lot of the time!) would be:
(e) Another exception is thrown to me (whether SSHException or IOException or something else), quickly, this exception might or might not wrap the InterruptedException, but the thread will always be in an interrupted state.
This is what I've seen elsewhere and what is normally suggested e.g. in Java Concurrency in Practice (good public summary at [1]).
It looks like there are just a few places that a declared InterruptedException is caught, and it would be straightforward to set the interrupted flag in those places. This seems like the right behaviour. However there might be nuances with the KeepAlive and Reader threads (they rely on thread interrupts; I think to cause them to exit which would be fine, but not 100% sure).
WDYT?