Multiple executions in ClientHttpRequestInterceptor cause next interceptors in chain not being called

[sinuhepop]

Affects: 6.1.3

In a couple of ClientHttpRequestInterceptor I made I needed to potentially execute given request more than once. Examples:

• RetryInterceptor: if some errors are raised making the call, retry again N times
• OAuthInterceptor: aside from refreshing token on expiration time, when a 401 is received I want to refresh token and execute again the same call

They are working correctly, but the problem is that any interceptors registered after these ones are only called on first execution. This is due to InterceptingClientHttpRequest.InterceptingRequestExecution:

public ClientHttpResponse execute(HttpRequest request, byte[] body) throws IOException {
	if (this.iterator.hasNext()) {
		ClientHttpRequestInterceptor nextInterceptor = this.iterator.next();
		return nextInterceptor.intercept(request, body, this);
	} else { ... }
}

So, the second time I call execution.execute(), this iterator keeps advancing and the next interceptor is skipped. IMHO this behaviour is not to be expected (nor documented), so I consider it a bug.

[quaff]

InterceptingRequestExecution seems to be one-off, why would you call it multiple times?

[sinuhepop]

@quaff I mentioned a two examples (RetryInterceptor and OAuthInterceptor) of behaviours that I thought ClientHttpRequestInterceptor was a good fit. In fact, they are working correctly (calls are made, connections are closed [some by me]) but there is this side effect.

If interceptors are intended to be one-off, as you said, I think it should be documented.

[bclozel]

I think this is by design. The Javadoc of the intercept method describes what an interceptor is supposed to do. This is very similar to a Servlet Filter / FilterChain arrangement where calling the next element in the chain means you're advancing in the chain - you are not supposed to call it several times.

I can see use cases where this behavior would be useful, and changing the behavior here should not break existing interceptors since they would only call it once. We could change InterceptingClientHttpRequest to ensure that the chain is stateless.

This has been the default behavior since Spring Framework 3.1 so I'm still hesitant to consider this change. What do you think @poutsma ?

[poutsma]

Indeed, this behavior has been there since the interceptors have been introduced, and—as you indicated—changing it would break backward compatibility.

[bclozel]

We have revisited this decision for 7.0, see #34169