ForgivingExceptionHandler swallows Errors

[vkochnev]

Several methods defined in ExceptionHandler interface accept Throwable as one of arguments. ForgivingExceptionHandler and DefaultExceptionHandler (which is used as a default ExceptionHandler implementation) as one of it's descendants effectively ignore any encountered Errors leaving application in abnormal state, which is strongly discouraged according to Error's description. Maybe it would be more correct to re-throw catched Errors after ExceptionHandler tried to log them or catch clauses should not even rely on ExceptionHandler to re-throw Errors.

[michaelklishin]

It would be a significant breaking change. You can always use any ExceptionHandler implementation you care to develop.

[vkochnev]

Sorry, but I honestly cannot imagine situation when anyone would rely on Errors to be ignored. Exceptions yes totally, but not Errors.

[vkochnev]

Consider similar discussion on spring-amqp https://github.com/spring-projects/spring-amqp/issues/1258

[acogoluegnes]

We could add a JavaErrorHandler to the existing ExceptionHandler implementations. The question is what to do in the default implementation of this "sub-handler".

We can System.exit(int) in the default implementation for the next major release (6.0) and just have a pass-through in the 5.x.

Or we can do the System.exit(int) in the current major release. This is what Spring AMQP did. Any feedback on this change @garyrussell @artembilan?

[garyrussell]

This is what Spring AMQP did.

Actually, we did the former; a second commit for that issue changed it to a no-op in 2.2.x

-       private OOMHandler oOMHandler = error -> System.exit(99);
+       private JavaLangErrorHandler javaLangErrorHandler = error -> { };

https://github.com/spring-projects/spring-amqp/blob/c96418f0f60659d9589ebfd45b44529958557f3e/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/AbstractMessageListenerContainer.java#L249

and exit the jvm in the next release 2.3.0

https://github.com/spring-projects/spring-amqp/blob/447a6964a971e6922dedf467927a6d0664de1635/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/AbstractMessageListenerContainer.java#L251

[acogoluegnes]

OK, thanks @garyrussell. So you chose to introduce the breaking change between 2.2 and 2.3. Is this something you usually do between minor versions?

[garyrussell]

Yes, if the problem is serious enough. At this stage of the project's maturity, major versions are far apart.

[acogoluegnes]

OK, thanks for the feedback @garyrussell. WDYT @michaelklishin?

[michaelklishin]

@acogoluegnes we can do the same thing and even bump major version. I wouldn't mind that.

[acogoluegnes]

I'd like to include several changes in 6.0 (#418, #432, #608), and even why not deprecate ConnectionFactory in favor of what comes out of #608, so it's a bit of work.

We can add a JavaErrorHandler to the existing exception handlers and make it a no-op for 5.x and System.exit(int) for 6.0.

[vkochnev]

I've done some search around the repo and discovered a few more suspicious Throwable catches also ignoring Errors https://github.com/rabbitmq/rabbitmq-java-client/blob/e32bcbb2824f7616a13acd5827a87ca92e54f08f/src/main/java/com/rabbitmq/client/impl/nio/NioLoop.java#L167 https://github.com/rabbitmq/rabbitmq-java-client/blob/e32bcbb2824f7616a13acd5827a87ca92e54f08f/src/main/java/com/rabbitmq/tools/jsonrpc/JsonRpcServer.java#L182 https://github.com/rabbitmq/rabbitmq-java-client/blob/ebc3e2929988dbd0b1e1421a19bfb14432681176/src/main/java/com/rabbitmq/client/impl/ChannelManager.java#L156 Would it be more appropriate to file them separately and link all together?