Skip to content
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

Inconsistent behaviour on Thread.interrupt() #800

Closed
ahgittin opened this issue Aug 2, 2022 · 2 comments
Closed

Inconsistent behaviour on Thread.interrupt() #800

ahgittin opened this issue Aug 2, 2022 · 2 comments

Comments

@ahgittin
Copy link
Contributor

ahgittin commented Aug 2, 2022

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?

@ahgittin
Copy link
Contributor Author

ahgittin commented Aug 2, 2022

I've submitted #801 with the changes so you can evaluate them. Happy to do more testing with them in the wild if you'd like.

@hierynomus
Copy link
Owner

Merged it in. I'll consider this issue closed. Feel free to reopen if I assumed wrongly.

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

No branches or pull requests

2 participants