Sending request with Jetty HTTP client: catching InterruptedException or not ?

[lolodomo]

@octa22 : I was asking myself if it is really useful to catch InterruptedException to then call

Thread.currentThread().interrupt();

You are doing that just to change the thing status.
But if the thread is interrupted, we can imagine that is for a severe reason and the thing handler will be certainly destroyed when it occurs.

My feeling is that InterruptedException should not be caught.
WDYT ? What was you reason to do that ?

[lolodomo]

Interesting, I checked how I handled exceptions in the remote openHAB binding when sending a Jetty HTTP request and I see that I caught Exception, meaning at the same time TimeoutException and InterruptedException. Doing that way, I am masking the interrupt !
Your approach might be the correct one, @octa22
It would be interesting to get the advice from an expert and then check and fix all bindings using the Jetty HTTP client.

@openhab/add-ons-maintainers for advice. Should InterruptedException be caught when calling request.send(); and if it should, should we call Thread.currentThread().interrupt(); ? What is the proper pattern to handle exceptions triggered by request.send(); ?

[fwolter]

I see lots of code catching Exception in reviews. The resulting problems are exactly like @lolodomo describes. Additionally, all unchecked exceptions like RuntimeException are cought, which might not be the intention. I try to throw InterruptedExceptions as high as possible. Thread.currentThread().interrupt() is not necessary in most cases, then.

[lolodomo]

I just checked few bindings and each one is doing differently.
The ones that thrown InterruptedException to a higher level exist but are a minority.
In most cases, the 3 exceptions are just caught, including InterruptedException. Some bindings, like Somfy Tahoma, are calling Thread.currentThread().interrupt();.

If we would like to throw this exception to a higher level, the change is more difficult, it would require to change the method calling request.send() but also all methods calling it.

At first steps, we could change:

1. all bindings just catching with Exception for the call to request.send() to replace it by each of the 3 exceptions triggered by the call. My remote openHAB binding is an example to fix.
2. all binding catching InterruptedException triggered by the call to request.send() to add a call to Thread.currentThread().interrupt(); if missing.

WDYT ?

[fwolter]

I'm fine with that.

You could combine the catch clause for TimeoutException and ExecutionException with the | operator.

[lolodomo]

More generally in relation to your previous remark, I found 625 catch of Exception !!!
I have not yet any idea how Exception catching are relative to Jetty HTTP requests but it should not be too much.

PS: I also found 7 catch of Throwable. Maybe these ones should be updated. Concerned bindings: elerotransmitterstick, homematic and zway.

org.openhab.binding.elerotransmitterstick/src/main/java/org/openhab/binding/elerotransmitterstick/internal/stick/TransmitterStick.java:            } catch (Throwable t) {
org.openhab.binding.homematic/src/main/java/org/openhab/binding/homematic/internal/discovery/HomematicDeviceDiscoveryService.java:                } catch (Throwable ex) {
org.openhab.binding.zway/src/main/java/org/openhab/binding/zway/internal/handler/ZWayBridgeHandler.java:            } catch (Throwable t) {
org.openhab.binding.zway/src/main/java/org/openhab/binding/zway/internal/handler/ZWayDeviceHandler.java:            } catch (Throwable t) {
org.openhab.binding.zway/src/main/java/org/openhab/binding/zway/internal/handler/ZWayDeviceHandler.java:                        } catch (Throwable t) {
org.openhab.binding.zway/src/main/java/org/openhab/binding/zway/internal/handler/ZWayZAutomationDeviceHandler.java:                    } catch (Throwable t) {
org.openhab.binding.zway/src/main/java/org/openhab/binding/zway/internal/handler/ZWayZWaveDeviceHandler.java:                    } catch (Throwable t) {

[lolodomo]

519 catch of InterruptedException found but I assume lot of them are relative to call to Thread.sleep (269 calls)..

[fwolter]

I peeked into the 3 bindings catching Throwable and none of them seem to have a valid reason.

Checkstyle should generates a warning if Throwable is cought.

InterruptedException can also handled wrongly when used with Thread.sleep(). It's quite similar to the Jetty usage...

[lolodomo]

Closing that as you have the answer.