ConcurrencyThrottleSupport and thread interruption

[ooraini]

ConcurrencyThrottleSupport handles interruption when waiting on its concurrencyCondition by re-interrupting the thread and throwing an IllegalStateException. This makes things slightly awkward to handle up the stack.

Suggestion: re-interrupt but don't throw IllegalStateException, instead return normally and let higher level clients handle it. In the case of ConcurrencyThrottleInterceptor it would do this:

@Override
	public @Nullable Object invoke(MethodInvocation methodInvocation) throws Throwable {
		beforeAccess();
                if (Thread.interrupted()) { // clear
                    throw new InterruptedException(); // not my problem
                }
		try {
			return methodInvocation.proceed();
		}
		finally {
			afterAccess();
		}
	}

As is right now it makes things awkward to deal with. I just wrote the following code:

catch (IllegalStateException e) {
  if (Thread.currentThread().isInterrupted() && Thread.interrupted()) { // clear; VERY IMPORTANT
      interrupted = true;
  }
  // handle e
}

I think this is a bug...?

[jhoeller]

The problem there is that we cannot propagate an InterruptedException since that is checked, and we cannot return normally either if the concurrency limit still does not allow for entering. So we have to re-interrupt and abort with some unchecked exception. While I agree that IllegalStateException is not ideal, it's not obvious what we should use instead. We could introduce a custom dedicated exception type for that scenario, I suppose.

[ooraini]

There is no reasonable way to implement ConcurrencyLimit and Retryable or any other aspect that will block on j.u.concurrent primitive(or Thread.sleep) without dealing with InterruptedException. For ConcurrencyLimit the only sensible thing to do is to propagate(sneaky throw).

It seems to me that Spring's proxy implementation wraps undeclared checked exception in an UndeclaredThrowableException. I'm not sure why... but it's better than an IllegalStateException.

Ideally it should really be InterruptedException. Let's imagine the following flow:

• HTTP request -> Servlet, Spring MVC -> @ConcurrencyLimit method
• method blocks
• request is canceled
• IllegalStateException(or UndeclaredThrowableException?)

UndeclaredThrowableException is definitely better. With the recommendation to users to declare such methods with throws InterruptedException

[jhoeller]

In general, data access methods and business logic methods are not meant to put the onus of enforced InterruptedException handling on the caller - which would imply mandatory action on the user side, even if just for re-interrupting the current thread. Core I/O handling is not designed like that, leading to an InterruptedIOException which just indicates termination triggered by an interrupt. The JDBC, JPA and JMS APIs are not designed like that either, and neither is Spring's recommended data access and messaging arrangement. Immediate re-interruption of the thread followed by a custom exception is the common approach.

In terms of our @ConcurrencyLimit and @Retryable design, the local blocking is just a temporary measure that essentially gets short-cut when the thread gets interrupted. This is unexpected in terms of the back-off algorithm there and leads to a re-interrupt with a rejection exception or an aborted retry attempt where the original user exception gets propagated instead. What we can definitely improve for @ConcurrencyLimit (and also for the use of concurrency throttling in our TaskExecutor arrangement) is to throw an InvocationRejectedException (or TaskRejectedException where applicable), aligning with our recently introduced configurable policy that allows for immediate rejection instead of blocking (see #36109). I'll resolve the issue that way for 7.0.4.

If you absolutely need to act on the interrupted status of a deep-stack method invocation, you'll have to catch and re-throw RuntimeException which allows for checking Thread.currentThread().isInterrupted() against potential interruption scenarios coming out of any API used underneath. As of 7.0.4, you may catch InvocationRejectedException to narrow it to Spring's concurrency limiting but even there you'll have to check the interruption status in order to differentiate it from immediate rejection scenarios.