ExpressionEvaluatingRequestHandlerAdvice sends mes

I had asked few doubts in the comments section of this thread.

While debugging the application I noticed some possible code errors. Are they bugs or features?

In ExpressionEvaluatingRequestHandlerAdvice::evaluateSuccessExp‌ression, the exception is thrown after AdviceMessage is sent to SuccessChannel. This causes Exception payload in it. Shouldn't the method first throw the exception if there is one?
In the same class, propagateOnSuccessEvaluationFailures is 'false' by default which causes not to throw Exceptions. That rather negates purpose of FailChannel. I set it to 'true' externally. Can you please explain thought process behind these?

Gary has answered,

I don't understand your point 1. For point 2., generally, an error handler handles the error and there is no need for the caller to know there was a problem. If there is, then generally, the error handler flow should throw a new exception to indicate to the caller the error handler has been involved. The boolean is for the rare instance that you want to handle the error and also throw the original exception to the caller.

Instead of cluttering that thread, we can discuss the points in here.

I think the explanation for point 2 is applicable for trapException.

To make my point clear, I have copied part of ExpressionEvaluatingRequestHandlerAdvice.

protected Object doInvoke(ExecutionCallback callback, Object target, Message<?> message) throws Exception {
        try {
            Object e = callback.execute();
            if(this.onSuccessExpression != null) {
                this.evaluateSuccessExpression(message);
            }

            return e;
        } catch (Exception var7) {
            Exception actualException = this.unwrapExceptionIfNecessary(var7);
            if(this.onFailureExpression != null) {
                Object evalResult = this.evaluateFailureExpression(message, actualException);
                if(this.returnFailureExpressionResult) {
                    return evalResult;
                }
            }

            if(!this.trapException) {
                throw actualException;
            } else {
                return null;
            }
        }
    }

    private void evaluateSuccessExpression(Message<?> message) throws Exception {
        boolean evaluationFailed = false;

        Object evalResult;
        try {
            evalResult = this.onSuccessExpression.getValue(this.prepareEvaluationContextToUse((Exception)null), message);
        } catch (Exception var5) {
            evalResult = var5;
            evaluationFailed = true;
        }

        if(this.successChannel == null && this.successChannelName != null && this.getChannelResolver() != null) {
            this.successChannel = (MessageChannel)this.getChannelResolver().resolveDestination(this.successChannelName);
        }

        if(evalResult != null && this.successChannel != null) {
            AdviceMessage resultMessage = new AdviceMessage(evalResult, message);
            this.messagingTemplate.send(this.successChannel, resultMessage);
        }

        if(evaluationFailed && this.propagateOnSuccessEvaluationFailures) {
            throw (Exception)evalResult;
        }
    }

    private Object evaluateFailureExpression(Message<?> message, Exception exception) throws Exception {
        Object evalResult;
        try {
            evalResult = this.onFailureExpression.getValue(this.prepareEvaluationContextToUse(exception), message);
        } catch (Exception var6) {
            evalResult = var6;
            this.logger.error("Failure expression evaluation failed for " + message + ": " + var6.getMessage());
        }

        if(this.failureChannel == null && this.failureChannelName != null && this.getChannelResolver() != null) {
            this.failureChannel = (MessageChannel)this.getChannelResolver().resolveDestination(this.failureChannelName);
        }

        if(evalResult != null && this.failureChannel != null) {
            ExpressionEvaluatingRequestHandlerAdvice.MessageHandlingExpressionEvaluatingAdviceException messagingException = new ExpressionEvaluatingRequestHandlerAdvice.MessageHandlingExpressionEvaluatingAdviceException(message, "Handler Failed", this.unwrapThrowableIfNecessary(exception), evalResult);
            ErrorMessage resultMessage = new ErrorMessage(messagingException);
            this.messagingTemplate.send(this.failureChannel, resultMessage);
        }

        return evalResult;
    }

In doInvoke, evaluateFailureExpression is called if callback.execute() or evaluateSuccessExpression throws exception.

Now in evaluateSuccessExpression, if there is any error while evaluating SuccessExpression, the Exception is stored in evalResult. This is sent to SuccessChannel if it is configured. If propagateOnSuccessEvaluationFailures is set to true then only evaluateSuccessExpression throws error. As it is set to 'false' by default, evalResult is not thrown and consequently not caught by evaluateFailureExpression.

Even if propagateOnSuccessEvaluationFailures needs to be set to 'false' by other requirements, the check to throw exception should be before sending the result to SuccessChannel.

I think you have the point. Please, raise a JIRA on the matter and we will revise that block of logic. In general I agree that it would be better to check for rethrow first of all and only after that send that exception to the successChannel.

Unfortunately there is no workaround for this (unless your own solution) and we can't fix it immediately because it is going to be behavior changing - someone might really rely on this status quo.

I even think that we may to revise the default for the propagateOnSuccessEvaluationFailures since you have point here as well. But again: we can make it only in the next 5.1 version.