After-completion callback not triggered for custom Throwable subclass [SPR-14329]

[spring-projects-issues]

Ben Heilers opened SPR-14329 and commented

Our team observed this, although I admit it is probably a rare situation.

To summarize, the JDBC connection is never released if:

• an exception is thrown in the controller method
• a second exception is thrown during handling of the first exception
• attempting to log that second exception causes a third exception to be thrown
• the third exception directly extends Throwable, not Exception or Error

I reproduced it with just small changes to the project at https://github.com/spring-projects/spring-boot/tree/master/spring-boot-samples/spring-boot-sample-data-jpa.

First, I changed the line in the controller method from "Bath" to "Foo", below:

@GetMapping("/")
@ResponseBody
@Transactional(readOnly = true)
public String helloWorld() {
    return this.cityService.getCity("Foo", "UK").getName();
}

This causes getCity() to return null, so the method throws an NPE.

So far, everything is okay, the exception is caught here, and the JDBC connection is still released within the call to triggerAfterCompletion().

But we really wanted a custom response HTTP status of 400 instead of 500, so we added:

@ResponseStatus(value=HttpStatus.BAD_REQUEST)
@ExceptionHandler(Exception.class)
public void handleException(Exception ex) {
   //TODO: some more handling logic to come later
}

Now the status is 500, but we still wanted some special logic in that exception handler for adding a log message.

Eventually, we observed now that in some scenarios that handling logic ends up throwing an second exception while trying to handle the exception thrown in the controller method.

This isn't so bad, because secondary exceptions thrown during exception handling are caught and handled here.

But we observed that the second exception is a custom class, that had an override of some of the methods in Throwable ... and those overrides in turn threw a third exception.

This also wouldn't be so bad by itself, because there is logic here to still end up unreleasing the JDBC connection in that scenario, as long as the exception sublcasses either Exception or Error.

Unfortunately, we found in our case this custom exception class was directly extending Throwable.

This means the JDBC connection is not released.

The below is a bit contrived, but it proves the point:

	@ResponseStatus(value=HttpStatus.BAD_REQUEST)
	@ExceptionHandler(Exception.class)
	public void handleException(Exception ex) {
		throwUnchecked(new MyException());
	}

	public static class MyException extends Throwable {
		@Override
		public String getMessage() {
			SampleController.throwUnchecked(this);
			return "";
		}
	}

	public static void throwUnchecked(Throwable e) {
		SampleController.<RuntimeException>throwAny(e);
	}

	@SuppressWarnings("unchecked")
	private static <E extends Throwable> void throwAny(Throwable e) throws E {
		throw (E) e;
	}

While I admit this is probably a rare edge case, this is actually a security vulnerability, since unreleased connections can be used as an attack vector. And there is nothing inapprorpiate about subclassing Throwable, or overriding methods like getMessage() to do custom logic there.

NOTE: if you are on spring 4.3, the logging level is debug, where it used to be error, so this whole issue is only reproducible if you also add the following to resources/application.properties:

logging.level.org.springframework.web.servlet.mvc.method.annotation=debug

---

Affects: 4.2.6, 4.3 RC2

Issue Links:

• @ExceptionHandler cannot handler java.lang.Error despite the annotation accept ? extends Throwable [SPR-11106] #15732 @ExceptionHandler cannot handler java.lang.Error despite the annotation accept ? extends Throwable
• Consistent Error handling in MessageChannel implementations [SPR-14323] #18895 Consistent Error handling in MessageChannel implementations
• Clarify rollback-on consistency between DefaultTransactionAttribute and TransactionTemplate [SPR-14994] #19560 Clarify rollback-on consistency between DefaultTransactionAttribute and TransactionTemplate

Backported to: 4.2.7

[spring-projects-issues]

Juergen Hoeller commented

Generally speaking, it is rather awkward to throw custom Throwable subclasses. They have no proper definition in the Java language and cannot be propagated unless a signature declares throws Throwable which our MVC contracts do not do, therefore DispatcherServlet doesn't guard against them. We do catch them deeper down the call stack, turning them into IllegalStateExceptions, but did not expect them to occur higher up anymore.

I understand that there are compiler tricks such as pretending a RuntimeException and other corner cases where a Throwable can be propagated. So we'll have to be extra-defensive there, and I'll pick this up for 4.3 and 4.2.7 from that perspective. However, with such custom Throwable classes in your codebase, you're asking for trouble: I'm not sure other frameworks can handle them properly or are even willing to consider this a valid case.

[spring-projects-issues]

Juergen Hoeller commented

I've made DispatcherServlet, DispatcherPortlet and also TransactionTemplate particularly defensive against custom Throwable subclasses now. We nevertheless strongly recommend not to use custom Throwable subclasses but rather just custom (Runtime)Exception subclasses.

[spring-projects-issues]

Ben Heilers commented

Thanks very much, for what it's worth, the exception is actually in a 3rd party library, not our code.

[spring-projects-issues]

Juergen Hoeller commented

Good point, if it's in code you don't control, defensiveness on your end as well as on the framework's end is the only way out.

This will be available in the upcoming 4.3.0.BUILD-SNAPSHOT and 4.2.7.BUILD-SNAPSHOT in case you'd like to give it an early try.