Reconsider a default order value for default LoggingHandler subscribed into a default errorChannel by the framework

[nikhilvasaikar]

In what version(s) of Spring Integration are you seeing this issue?

5.4.3

Describe the bug

There is no consistency between DSL and XML config for the order of subscribers on the global errorChannel. The order attribute also does not cause any effect in the XML configuration.

To Reproduce

1. Add a exception throwing service-activator to the global errorChannel
2. Simulate a message dispatch to the global errorChannel

When the above is implemented using DSL, the behavior is as expected below. However, when implemented in XML, the exception throwing service-activator in invoked before the error logging handler, hence causing the error logging handler to be never invoked as both subscribers are invoked on the caller's thread. Setting order="100" on the service-activator XML declaration also has no effect.

Expected behavior

1. The default error logging handler attached to the global errorChannel should be invoked first and must log the error
2. The exception throwing service-activator must be invoked after, not before the error logging handler

Sample

A brief description and scenario under which this issue was encountered is available at this stackoverflow post.

[artembilan]

Well, the behavior is correct: I've just misled you a little bit.
See Ordered.getOrder() JavaDocs:

	/**
	 * Get the order value of this object.
	 * <p>Higher values are interpreted as lower priority. As a consequence,
	 * the object with the lowest value has the highest priority (somewhat
	 * analogous to Servlet {@code load-on-startup} values).
	 * <p>Same order values will result in arbitrary sort positions for the
	 * affected objects.
	 * @return the order value
	 * @see #HIGHEST_PRECEDENCE
	 * @see #LOWEST_PRECEDENCE
	 */
	int getOrder();

By default we use a LOWEST_PRECEDENCE for our MessageHandlers and that LoggingHandler subscribed into a default global errorChannel doesn't change anything with an order option. Therefore when we specify an order like 100 for our custom subscribers, we just make them more important than the rest, including that default LoggingHandler. So, currently the custom subscribers are always called before the custom if they have non a default order option.

With your Java DSL approach (IntegrationFlows.from("errorChannel")) you don't specify an order, therefore a subscriber comes to that errorChannel with a default LOWEST_PRECEDENCE order. And you were just lucky to get an expected order in your app.

I would say your order="100" just doesn't give us a way to compare apples with apples, therefore no bug on the framework side. Unless we have a wish to revise a default logic, at least for that LoggingHandler on the errorChannel to let it to be adjusted for any end-user use-case. For example specify it's order like LOWEST_PRECEDENCE/2, so there won't be necessary to configre a custom logger, but just an extract subscriber would have its order to configured like LOWEST_PRECEDENCE/2 + 1 - and you got an expected behavior to log before you re-throw.

I would treat it as a behavior change anyway, therefore if we agree with a feature, it is going to be scheduled for the next 6.0 version.

Meanwhile as a workaround for XML use-case, it is just enough to specify an extra <int:logging-channel-adapter channel="errorChannel" order="101"/> along side with your <int:service-activator channel="errorChannel" order="100">.

@garyrussell , WDYT?

[garyrussell]

Yes, we should leave a little room; but I think it should be closer to LOWEST_PRECEDENCE, maybe L_P - 100.

I think you mean he needs a logging handler at 99, not 101, to log first.

You should also define your own pub/sub channel errorChannel so we don't subscribe the default handler.

[artembilan]

Thanks, Gary, for feedback and correction.
True, it must be something like this:

<int:logging-channel-adapter channel="errorChannel" order="99"/>

<int:service-activator channel="errorChannel" order="100">

Yes, it is also better to use a custom error channel for each specific case and don't abuse a global one if you are intended to have some custom error handler in the end. This way everything is in your control. But yeah... according that SO thread, @nikhilvasaikar just doesn't want to do any custom error channel...

[nikhilvasaikar]

Thank You @artembilan and @garyrussell. The reason why I did not create a custom errorChannel, is because the framework already provides me with one and also with a logger attached to it. I thought, I should be able to just add another subscriber to it, since the default errorChannel is a Pub/Sub channel. Besides I only want to log and throw the exceptions (including from downstream flows) back to the SimpleMessageListenerContainer. Also, with default errorChannel I have the benefit of all downstream exceptions going to it, thus I don't have to configure any errorChannels on downstream flows.

May be the default errorChannel can use a default executor? Any errors from handlers on the default errorChannel can either go to a custom error channel (if mentioned) or to the caller thread. This way the default errorChannel can also be extensible to add custom handlers. Sorry, if this is going off-topic.

[artembilan]

Thank you for confirmation, @nikhilvasaikar , that we are on the same page with a compromise we are trying to find over here.

What you are saying about custom executor, handlers etc. is fully OK for custom error handlers.
It is really better to keep a default one as simple as possible and treat it as a last resort for all the unhandled exceptions or an intermediate state before the final business decision in the design.

As I said: with a custom, some error-specific handler subscribed to a default errorChannel it is easy to introduce a flaw into your system when many other flows rely on the same default errorChannel, but they don't expect to be broken because of custom handler not for their errors. That's why a custom error channel is a recommended way to go.

[garyrussell]

Changing the order of the default handler, leaving space for custom handlers, will solve the problem (in 6.0), before then, you will need to add your own errorChannel to get the behavior you desire.

But @artembilan is correct; the default errorChannel is for default behavior.

[nikhilvasaikar]

@garyrussell @artembilan I agree that we should keep application specific behavior separate from default behavior. I will add a custom errorChannel that handles application specific use cases.