Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CAMEL-17075 Moved initialisation from evaluate() to init() #6267

Closed
wants to merge 1 commit into from

Conversation

henka-rl
Copy link
Contributor

No description provided.

Copy link
Contributor

@oscerd oscerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this deserve to be done even in 3.11.x because it seems more a bug under load.

@davsclaus
Copy link
Contributor

I would assume there may have been a reason in the past for that double init in evaluate

Have you run all tests in core with this change?

@davsclaus
Copy link
Contributor

Ah you cannot do this as you do not know if the simple builder is used for expression or predicate, and a language can behave differently in one vs the other, and as such can cause a parsing exception in init

@henka-rl
Copy link
Contributor Author

I run all core tests and SimpleBuilderTest fails, but I would say that's because they don't call init() before calling evaluate().
If I look at the interfaces Predicate and Expression they both says that all initialization should be done in init() and should be thread safe:
"A predicate should be thread-safe and be able to evaluate concurrently by different threads with different exchanges. Any initialization logic should be done by the init(CamelContext) method which allows to prepare the predicate such as wiring in resources, pre-parsing and what else."

@henka-rl
Copy link
Contributor Author

henka-rl commented Oct 13, 2021

I tried to solve the issue with parsing exception in init like this:
` @OverRide
public T evaluate(Exchange exchange, Class type) {
if (expressionException == null) {
return expression.evaluate(exchange, type);
} else {
throw expressionException;
}
}

@Override
public boolean matches(Exchange exchange) {
    if (predicateException == null) {
        return predicate.matches(exchange);
    } else {
        throw predicateException;
    }
}

@Override
public void init(CamelContext context) {
    simple = context.resolveLanguage("simple");
    text = context.resolvePropertyPlaceholders(text);

    try {
        if (resultType != null) {
            Object[] properties = new Object[2];
            properties[0] = resultType;
            expression = simple.createExpression(text, properties);
        } else {
            expression = simple.createExpression(text);
        }
        expression.init(context);
    } catch (RuntimeCamelException e) {
        expressionException = e;
    }
    try {
        predicate = simple.createPredicate(text);
        predicate.init(context);
    } catch (RuntimeCamelException e) {
        predicateException = e;
    }
}`

But not all testcases pass anyway as it looks like init() is not always called before the first call to evaluate() or matches()

@davsclaus
Copy link
Contributor

Removed DataSonnetBuilder so its like all the other languages.

@davsclaus davsclaus closed this Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants