You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "rhuan080 (via GitHub)" <gi...@apache.org> on 2023/02/10 23:04:27 UTC
[GitHub] [camel] rhuan080 opened a new pull request, #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
rhuan080 opened a new pull request, #9332:
URL: https://github.com/apache/camel/pull/9332
Signed-off-by: Rhuan Rocha <rh...@gmail.com>
# Description
Fixing the issue [CAMEL-19014](https://issues.apache.org/jira/browse/CAMEL-19014)
# Target
- [x] I checked that the commit is targeting the correct branch (note that Camel 3 uses `camel-3.x`, whereas Camel 4 uses the `main` branch)
# Tracking
- [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it).
<!--
# *Note*: trivial changes like, typos, minor documentation fixes and other small items do not require a JIRA issue. In this case your pull request should address just this issue, without pulling in other changes.
-->
# Apache Camel coding standards and style
- [x] I checked that each commit in the pull request has a meaningful subject line and body.
<!--
If you're unsure, you can format the pull request title like `[CAMEL-XXX] Fixes bug in camel-file component`, where you replace `CAMEL-XXX` with the appropriate JIRA issue.
-->
- [x] I formatted the code using `mvn -Pformat,fastinstall install && mvn -Psourcecheck`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] essobedo commented on a diff in pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on code in PR #9332:
URL: https://github.com/apache/camel/pull/9332#discussion_r1109451751
##########
core/camel-support/src/main/java/org/apache/camel/support/builder/ExpressionBuilder.java:
##########
@@ -1577,74 +1578,122 @@ public String toString() {
* Returns an expression which returns the string concatenation value of the various
* expressions
*
+ * @param context the Camel context
* @param expressions the expression to be concatenated dynamically
* @return an expression which when evaluated will return the concatenated values
*/
- public static Expression concatExpression(final Collection<Expression> expressions) {
- return concatExpression(expressions, null);
+ public static Expression concatExpression(final CamelContext context, final Collection<Expression> expressions) {
+ return concatExpression(context, expressions, null);
}
/**
* Returns an expression which returns the string concatenation value of the various
* expressions
*
+ * @param context the Camel context
* @param expressions the expression to be concatenated dynamically
* @param description the text description of the expression
* @return an expression which when evaluated will return the concatenated values
*/
- public static Expression concatExpression(final Collection<Expression> expressions, final String description) {
- return new ExpressionAdapter() {
+ public static Expression concatExpression(final CamelContext context, final Collection<Expression> expressions, final String description) {
+
+ for (Expression expression : expressions) {
+ if(expression instanceof ConstantExpressionAdapter){
+ return concatExpressionOptimized(context, expressions, description);
+ }
+ }
- private Collection<Object> col;
+ return concatExpression(expressions,description);
+ }
+
+ /**
+ * Returns an expression which returns the string concatenation value of the various
+ * expressions
+ *
+ * @param expressions the expression to be concatenated dynamically
+ * @param description the text description of the expression
+ * @return an expression which when evaluated will return the concatenated values
+ */
+ private static Expression concatExpression(final Collection<Expression> expressions, final String description) {
+ return new ExpressionAdapter() {
@Override
public Object evaluate(Exchange exchange) {
StringBuilder buffer = new StringBuilder();
- if (col != null) {
- // optimize for constant expressions so we can do this a bit faster
- for (Object obj : col) {
- if (obj instanceof Expression) {
- Expression expression = (Expression) obj;
- String text = expression.evaluate(exchange, String.class);
- if (text != null) {
- buffer.append(text);
- }
- } else {
- buffer.append((String) obj);
- }
+ for (Expression expression : expressions) {
+ String text = expression.evaluate(exchange, String.class);
+ if (text != null) {
+ buffer.append(text);
}
+ }
+ return buffer.toString();
+ }
+
+ @Override
+ public void init(CamelContext context) {
+ boolean constant = false;
Review Comment:
Not needed
##########
core/camel-support/src/main/java/org/apache/camel/support/builder/ExpressionBuilder.java:
##########
@@ -1577,74 +1578,122 @@ public String toString() {
* Returns an expression which returns the string concatenation value of the various
* expressions
*
+ * @param context the Camel context
* @param expressions the expression to be concatenated dynamically
* @return an expression which when evaluated will return the concatenated values
*/
- public static Expression concatExpression(final Collection<Expression> expressions) {
- return concatExpression(expressions, null);
+ public static Expression concatExpression(final CamelContext context, final Collection<Expression> expressions) {
+ return concatExpression(context, expressions, null);
}
/**
* Returns an expression which returns the string concatenation value of the various
* expressions
*
+ * @param context the Camel context
* @param expressions the expression to be concatenated dynamically
* @param description the text description of the expression
* @return an expression which when evaluated will return the concatenated values
*/
- public static Expression concatExpression(final Collection<Expression> expressions, final String description) {
- return new ExpressionAdapter() {
+ public static Expression concatExpression(final CamelContext context, final Collection<Expression> expressions, final String description) {
+
+ for (Expression expression : expressions) {
+ if(expression instanceof ConstantExpressionAdapter){
+ return concatExpressionOptimized(context, expressions, description);
+ }
+ }
- private Collection<Object> col;
+ return concatExpression(expressions,description);
+ }
+
+ /**
+ * Returns an expression which returns the string concatenation value of the various
+ * expressions
+ *
+ * @param expressions the expression to be concatenated dynamically
+ * @param description the text description of the expression
+ * @return an expression which when evaluated will return the concatenated values
+ */
+ private static Expression concatExpression(final Collection<Expression> expressions, final String description) {
+ return new ExpressionAdapter() {
@Override
public Object evaluate(Exchange exchange) {
StringBuilder buffer = new StringBuilder();
- if (col != null) {
- // optimize for constant expressions so we can do this a bit faster
- for (Object obj : col) {
- if (obj instanceof Expression) {
- Expression expression = (Expression) obj;
- String text = expression.evaluate(exchange, String.class);
- if (text != null) {
- buffer.append(text);
- }
- } else {
- buffer.append((String) obj);
- }
+ for (Expression expression : expressions) {
+ String text = expression.evaluate(exchange, String.class);
+ if (text != null) {
+ buffer.append(text);
}
+ }
+ return buffer.toString();
+ }
+
+ @Override
+ public void init(CamelContext context) {
+ boolean constant = false;
+ for (Expression expression : expressions) {
+ expression.init(context);
+ }
+ }
+
+ @Override
+ public String toString() {
+ if (description != null) {
+ return description;
} else {
- for (Expression expression : expressions) {
+ return "concat(" + expressions + ")";
+ }
+ }
+ };
+ }
+
+ /**
+ * Returns an optimized expression which returns the string concatenation value of the various.
+ * expressions
+ *
+ * @param context the Camel context
+ * @param expressions the expression to be concatenated dynamically
+ * @param description the text description of the expression
+ * @return an expression which when evaluated will return the concatenated values
+ */
+ private static Expression concatExpressionOptimized(final CamelContext context, final Collection<Expression> expressions, final String description) {
+ Collection<Object> preprocessedExpression = new ArrayList<>(expressions.size());
+ for (Expression expression : expressions) {
+ if (expression instanceof ConstantExpressionAdapter) {
+ expression.init(context);
Review Comment:
I did not realize that for some `ConstantExpressionAdapter` we had to initialize it first to get its value, I don't know if it is acceptable to call `init` so early? @oscerd WDYT?
If it is not acceptable, the other way could be to use a volatile boolean field to know if the expression has been initialized or not and use the Double-checked locking approach to ensure that it is initialized once. Even if it adds a locking mechanism it is only during the initialization phase which happens only once per expression when properly used so the overhead is limited.
##########
core/camel-support/src/main/java/org/apache/camel/support/builder/ExpressionBuilder.java:
##########
@@ -1577,74 +1578,122 @@ public String toString() {
* Returns an expression which returns the string concatenation value of the various
* expressions
*
+ * @param context the Camel context
* @param expressions the expression to be concatenated dynamically
* @return an expression which when evaluated will return the concatenated values
*/
- public static Expression concatExpression(final Collection<Expression> expressions) {
- return concatExpression(expressions, null);
+ public static Expression concatExpression(final CamelContext context, final Collection<Expression> expressions) {
+ return concatExpression(context, expressions, null);
}
/**
* Returns an expression which returns the string concatenation value of the various
* expressions
*
+ * @param context the Camel context
* @param expressions the expression to be concatenated dynamically
* @param description the text description of the expression
* @return an expression which when evaluated will return the concatenated values
*/
- public static Expression concatExpression(final Collection<Expression> expressions, final String description) {
- return new ExpressionAdapter() {
+ public static Expression concatExpression(final CamelContext context, final Collection<Expression> expressions, final String description) {
+
+ for (Expression expression : expressions) {
+ if(expression instanceof ConstantExpressionAdapter){
+ return concatExpressionOptimized(context, expressions, description);
+ }
+ }
- private Collection<Object> col;
+ return concatExpression(expressions,description);
+ }
+
+ /**
+ * Returns an expression which returns the string concatenation value of the various
+ * expressions
+ *
+ * @param expressions the expression to be concatenated dynamically
+ * @param description the text description of the expression
+ * @return an expression which when evaluated will return the concatenated values
+ */
+ private static Expression concatExpression(final Collection<Expression> expressions, final String description) {
+ return new ExpressionAdapter() {
@Override
public Object evaluate(Exchange exchange) {
StringBuilder buffer = new StringBuilder();
- if (col != null) {
- // optimize for constant expressions so we can do this a bit faster
- for (Object obj : col) {
- if (obj instanceof Expression) {
- Expression expression = (Expression) obj;
- String text = expression.evaluate(exchange, String.class);
- if (text != null) {
- buffer.append(text);
- }
- } else {
- buffer.append((String) obj);
- }
+ for (Expression expression : expressions) {
+ String text = expression.evaluate(exchange, String.class);
+ if (text != null) {
+ buffer.append(text);
}
+ }
+ return buffer.toString();
+ }
+
+ @Override
+ public void init(CamelContext context) {
+ boolean constant = false;
+ for (Expression expression : expressions) {
+ expression.init(context);
+ }
+ }
+
+ @Override
+ public String toString() {
+ if (description != null) {
+ return description;
} else {
- for (Expression expression : expressions) {
+ return "concat(" + expressions + ")";
+ }
+ }
+ };
+ }
+
+ /**
+ * Returns an optimized expression which returns the string concatenation value of the various.
+ * expressions
+ *
+ * @param context the Camel context
+ * @param expressions the expression to be concatenated dynamically
+ * @param description the text description of the expression
+ * @return an expression which when evaluated will return the concatenated values
+ */
+ private static Expression concatExpressionOptimized(final CamelContext context, final Collection<Expression> expressions, final String description) {
+ Collection<Object> preprocessedExpression = new ArrayList<>(expressions.size());
+ for (Expression expression : expressions) {
+ if (expression instanceof ConstantExpressionAdapter) {
+ expression.init(context);
+ Object value = ((ConstantExpressionAdapter) expression).getValue();
+ preprocessedExpression.add(value.toString());
+ } else {
+ preprocessedExpression.add(expression);
+ }
+ }
+
+ return new ExpressionAdapter() {
+
+ private final Collection<Object> col = Collections.unmodifiableCollection(preprocessedExpression);
+
+ @Override
+ public Object evaluate(Exchange exchange) {
+ StringBuilder buffer = new StringBuilder();
+ for (Object obj : col) {
+ if (obj instanceof Expression) {
+ Expression expression = (Expression) obj;
String text = expression.evaluate(exchange, String.class);
if (text != null) {
buffer.append(text);
}
+ } else {
+ buffer.append((String) obj);
}
}
return buffer.toString();
}
@Override
public void init(CamelContext context) {
- boolean constant = false;
for (Expression expression : expressions) {
expression.init(context);
Review Comment:
Assuming that it is acceptable to call `init` when creating the expression, we could avoid calling init on expressions of type `ConstantExpressionAdapter` as they are already initialized
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1464867322
So we need a PR for camel-3.x branch and leave 3.20.x as-is.
And in 3.x we need this PR and the 2 commits I linked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] essobedo commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1427091598
Well, we don't necessarily need 2 classes, but the code will be cleaner and thus easier to maintain, it will then follow the Single-responsibility principle.
Regarding, the unit test, you can simply adapt what the OP provided as it helps to reproduce the bug.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] github-actions[bot] commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1435974144
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:warning: Please note that the changes on this PR may be **tested automatically**.
If necessary Apache Camel Committers may access logs and test results in the job summaries!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1426843927
I agree about the TDD, and I can add it here, but the project does not use TDD. The proof is we have a few tests in this module, but I agree we should add unit tests when submitting a PR and I can do that. However, I'm not sure the right way to solve that is by creating 2 classes. My point is the Expression object can be shared between threads and we have a cache of this object. Thus, in my opinion, an object that keeps states and works (or can work) inside a parallel context should be predicted concurrence problems. The best approach for me is to use an immutable object, however, if it is not possible I think we should consider using a lock or synchronized block.
> It if needs to be fixed, it is not the right way to fix it, you don't even need locks here. If you really want to fix it, you will have to rewrite the code of the concatExpression, to have 2 classes instead of one (for constants only and not only constants). The problem is with the case of the not only constants, for it, you simply need to set the content of col once for all while initializing the instance instead of doing it in the init method. Moreover, you would need to add a unit test to prove that the bug is really fixed, [see TDD](https://en.wikipedia.org/wiki/Test-driven_development).
Yes, I agree! I'm putting my point and analyzing your point. This PR is good to drive this discussion.
> This decision doesn't really belong to us, especially if we disagree, I guess that someone like @davsclaus or @oscerd could eventually decide. But, here, according to your stack trace, if we get this problem it is because the expressions are misused such that the threads spend their time to re-initialize the same expression over and over again instead of only once at startup as the framework does, I don't believe that for a production-ready code, we would really want this behavior, so it tends to prove that it is more a misuse than a bug but it is only my point of view, let's see what others think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1464867229
And this commit optimizes when there are only constant values
https://github.com/apache/camel/commit/f2a3cc20d07271cc2bfba26cb3ecdcf4c119866d
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1464863431
Okay this commit fixes the NPEs
https://github.com/apache/camel/commit/d6ca5fca99d528b5394113433c5f4e9a1b0b5296
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] essobedo commented on a diff in pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on code in PR #9332:
URL: https://github.com/apache/camel/pull/9332#discussion_r1109946937
##########
core/camel-support/src/main/java/org/apache/camel/support/builder/ExpressionBuilder.java:
##########
@@ -1577,74 +1578,122 @@ public String toString() {
* Returns an expression which returns the string concatenation value of the various
* expressions
*
+ * @param context the Camel context
* @param expressions the expression to be concatenated dynamically
* @return an expression which when evaluated will return the concatenated values
*/
- public static Expression concatExpression(final Collection<Expression> expressions) {
- return concatExpression(expressions, null);
+ public static Expression concatExpression(final CamelContext context, final Collection<Expression> expressions) {
+ return concatExpression(context, expressions, null);
}
/**
* Returns an expression which returns the string concatenation value of the various
* expressions
*
+ * @param context the Camel context
* @param expressions the expression to be concatenated dynamically
* @param description the text description of the expression
* @return an expression which when evaluated will return the concatenated values
*/
- public static Expression concatExpression(final Collection<Expression> expressions, final String description) {
- return new ExpressionAdapter() {
+ public static Expression concatExpression(final CamelContext context, final Collection<Expression> expressions, final String description) {
+
+ for (Expression expression : expressions) {
+ if(expression instanceof ConstantExpressionAdapter){
+ return concatExpressionOptimized(context, expressions, description);
+ }
+ }
- private Collection<Object> col;
+ return concatExpression(expressions,description);
+ }
+
+ /**
+ * Returns an expression which returns the string concatenation value of the various
+ * expressions
+ *
+ * @param expressions the expression to be concatenated dynamically
+ * @param description the text description of the expression
+ * @return an expression which when evaluated will return the concatenated values
+ */
+ private static Expression concatExpression(final Collection<Expression> expressions, final String description) {
+ return new ExpressionAdapter() {
@Override
public Object evaluate(Exchange exchange) {
StringBuilder buffer = new StringBuilder();
- if (col != null) {
- // optimize for constant expressions so we can do this a bit faster
- for (Object obj : col) {
- if (obj instanceof Expression) {
- Expression expression = (Expression) obj;
- String text = expression.evaluate(exchange, String.class);
- if (text != null) {
- buffer.append(text);
- }
- } else {
- buffer.append((String) obj);
- }
+ for (Expression expression : expressions) {
+ String text = expression.evaluate(exchange, String.class);
+ if (text != null) {
+ buffer.append(text);
}
+ }
+ return buffer.toString();
+ }
+
+ @Override
+ public void init(CamelContext context) {
+ boolean constant = false;
+ for (Expression expression : expressions) {
+ expression.init(context);
+ }
+ }
+
+ @Override
+ public String toString() {
+ if (description != null) {
+ return description;
} else {
- for (Expression expression : expressions) {
+ return "concat(" + expressions + ")";
+ }
+ }
+ };
+ }
+
+ /**
+ * Returns an optimized expression which returns the string concatenation value of the various.
+ * expressions
+ *
+ * @param context the Camel context
+ * @param expressions the expression to be concatenated dynamically
+ * @param description the text description of the expression
+ * @return an expression which when evaluated will return the concatenated values
+ */
+ private static Expression concatExpressionOptimized(final CamelContext context, final Collection<Expression> expressions, final String description) {
+ Collection<Object> preprocessedExpression = new ArrayList<>(expressions.size());
+ for (Expression expression : expressions) {
+ if (expression instanceof ConstantExpressionAdapter) {
+ expression.init(context);
Review Comment:
> About the lock, the problem is the `evaluate` method being called concurrently with the `init`calling. Thus, I think the lock should be put at the `evaluate` method as well. However, we came back to the penalty we said before.
To be thread-safe, we have to consider the state changes and the visibility of those changes. Here only the `init` method affects the state and the `init` method is always called before `evaluate` so applying a double-check locking approach in the `init` method is good enough as it ensures that the state is changed only once at the very first call and that all threads thanks to a volatile read or synchronized block will always have the right state before calling `evaluate`. I hope it is clear enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1430470487
The test uses components that are not available in this module. Thus, I created the test using what is available inside the support module. I have reproduced the issue with my test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus closed pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus closed pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
URL: https://github.com/apache/camel/pull/9332
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1429911577
Hi @oscerd,
Yes, I agree the way this was figured out is a corner case. However the Expression is used inside a parallel and multithreaded context, but it is not immutable nor thread-safe. Thus, I think we can use an immutable implementation to avoid problems. As it is a good practice to parallel processes, I think it is an opportunity to improve it. I agreed with @essobedo about the lock and the tradeoff of this solution with lock, however, using an immutable implementation we don't have this tradeoff and we avoid a problem about concurrency or rance condition.
What do you think about?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] oscerd commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "oscerd (via GitHub)" <gi...@apache.org>.
oscerd commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1430004989
Using immutable classes sounds good and I'm +1 on that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] essobedo commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1430458256
> Hi,
>
> I added the unit test. Now, I'll rewrite the concatExpression to be immutable. @essobedo and @oscerd If you have questions or improvements about the unit test, please, tell me :-)
I don't understand why you don't use the test case provided by the OP?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1427062308
Understood and I agree. However, I think we don't need 2 classes, but just rewrite the concatExpression to have 1 class immutable. What do you think about it?
In the case of 2 classes, we will need to define when using immutable e when using mutable. For me, it is an unneeded complex.
> > The best approach for me is to use an immutable object, however, if it is not possible I think we should consider using a lock or synchronized block.
>
> Actually, immutability is the idea behind what I proposed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] essobedo commented on a diff in pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on code in PR #9332:
URL: https://github.com/apache/camel/pull/9332#discussion_r1108919438
##########
core/camel-core/src/test/java/org/apache/camel/builder/ExpressionBuilderConcurrencyTest.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.builder;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.camel.ContextTestSupport;
+import org.apache.camel.Exchange;
+import org.apache.camel.Processor;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.model.language.SimpleExpression;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class ExpressionBuilderConcurrencyTest extends ContextTestSupport {
+
+ @Test
+ public void testConcatExpressionConcurrency() throws Exception {
+ MockEndpoint mockWithFailure = getMockEndpoint("mock:result");
+ mockWithFailure.expectedMessageCount(102);
+ mockWithFailure.assertIsSatisfied();
+ //assertMockEndpointsSatisfied();
Review Comment:
Could be removed
##########
core/camel-core/src/test/java/org/apache/camel/builder/ExpressionBuilderConcurrencyTest.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.builder;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.camel.ContextTestSupport;
+import org.apache.camel.Exchange;
+import org.apache.camel.Processor;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.model.language.SimpleExpression;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class ExpressionBuilderConcurrencyTest extends ContextTestSupport {
+
+ @Test
+ public void testConcatExpressionConcurrency() throws Exception {
+ MockEndpoint mockWithFailure = getMockEndpoint("mock:result");
+ mockWithFailure.expectedMessageCount(102);
+ mockWithFailure.assertIsSatisfied();
+ //assertMockEndpointsSatisfied();
+ List<Exchange> exchanges = mockWithFailure.getExchanges();
+ exchanges.stream()
+ .forEach(exchange -> Assertions.assertTrue(exchange.getMessage()
+ .getHeader("#CustomHeader", String.class)
+ .equals("This is a test a with startLabel: `Document` endLabel: `Document` and label: `ALabel`")));
Review Comment:
For clarity, it should rather be a simple `for` loop. Moreover, please use `assertEquals` instead.
##########
core/camel-core/src/test/java/org/apache/camel/builder/ExpressionBuilderConcurrencyTest.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.builder;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.camel.ContextTestSupport;
+import org.apache.camel.Exchange;
+import org.apache.camel.Processor;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.model.language.SimpleExpression;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class ExpressionBuilderConcurrencyTest extends ContextTestSupport {
+
+ @Test
+ public void testConcatExpressionConcurrency() throws Exception {
+ MockEndpoint mockWithFailure = getMockEndpoint("mock:result");
+ mockWithFailure.expectedMessageCount(102);
+ mockWithFailure.assertIsSatisfied();
+ //assertMockEndpointsSatisfied();
+ List<Exchange> exchanges = mockWithFailure.getExchanges();
+ exchanges.stream()
+ .forEach(exchange -> Assertions.assertTrue(exchange.getMessage()
+ .getHeader("#CustomHeader", String.class)
+ .equals("This is a test a with startLabel: `Document` endLabel: `Document` and label: `ALabel`")));
+ }
+
+ @Override
+ protected RouteBuilder createRouteBuilder() throws Exception {
+
+ return new RouteBuilder() {
+ Map body = new HashMap() {
+ {
+ put("label", "ALabel");
+ put("startLabel", "Document");
+ put("endLabel", "Document");
+ }
+ };
Review Comment:
Let's use, `Map.of("label", "ALabel", "startLabel", "Document", "endLabel", "Document")` instead
##########
core/camel-core/src/test/java/org/apache/camel/builder/ExpressionBuilderConcurrencyTest.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.builder;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.camel.ContextTestSupport;
+import org.apache.camel.Exchange;
+import org.apache.camel.Processor;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.model.language.SimpleExpression;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class ExpressionBuilderConcurrencyTest extends ContextTestSupport {
+
+ @Test
+ public void testConcatExpressionConcurrency() throws Exception {
+ MockEndpoint mockWithFailure = getMockEndpoint("mock:result");
+ mockWithFailure.expectedMessageCount(102);
Review Comment:
Let's put `100` to avoid making it sound like a magic number
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1433660681
Hi @essobedo, I'll fix the unit tests. Thanks a lot!
I have committed the refractory of the concatExpression and created one method to build the optimized expression (immutable) and the unoptimized expression. Let me know if you have any questions about the solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] essobedo commented on a diff in pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on code in PR #9332:
URL: https://github.com/apache/camel/pull/9332#discussion_r1110927054
##########
core/camel-support/src/main/java/org/apache/camel/support/builder/ExpressionBuilder.java:
##########
@@ -1593,59 +1594,109 @@ public static Expression concatExpression(final Collection<Expression> expressio
* @return an expression which when evaluated will return the concatenated values
*/
public static Expression concatExpression(final Collection<Expression> expressions, final String description) {
- return new ExpressionAdapter() {
- private Collection<Object> col;
+ for (Expression expression : expressions) {
+ if(expression instanceof ConstantExpressionAdapter){
+ return concatExpressionOptimized(expressions, description);
+ }
+ }
+
+ return concatExpressionUnoptimized(expressions,description);
+ }
+
+ /**
+ * Returns an expression which returns the string concatenation value of the various
+ * expressions
+ *
+ * @param expressions the expression to be concatenated dynamically
+ * @param description the text description of the expression
+ * @return an expression which when evaluated will return the concatenated values
+ */
+ private static Expression concatExpressionUnoptimized(final Collection<Expression> expressions, final String description) {
+ return new ExpressionAdapter() {
@Override
public Object evaluate(Exchange exchange) {
StringBuilder buffer = new StringBuilder();
- if (col != null) {
- // optimize for constant expressions so we can do this a bit faster
- for (Object obj : col) {
- if (obj instanceof Expression) {
- Expression expression = (Expression) obj;
- String text = expression.evaluate(exchange, String.class);
- if (text != null) {
- buffer.append(text);
- }
- } else {
- buffer.append((String) obj);
- }
+ for (Expression expression : expressions) {
+ String text = expression.evaluate(exchange, String.class);
+ if (text != null) {
+ buffer.append(text);
}
+ }
+ return buffer.toString();
+ }
+
+ @Override
+ public void init(CamelContext context) {
+ for (Expression expression : expressions) {
+ expression.init(context);
+ }
+ }
+
+ @Override
+ public String toString() {
+ if (description != null) {
+ return description;
} else {
- for (Expression expression : expressions) {
+ return "concat(" + expressions + ")";
+ }
+ }
+ };
+ }
+
+ /**
+ * Returns an optimized expression which returns the string concatenation value of the various.
+ * expressions
+ *
+ * @param expressions the expression to be concatenated dynamically
+ * @param description the text description of the expression
+ * @return an expression which when evaluated will return the concatenated values
+ */
+ private static Expression concatExpressionOptimized(final Collection<Expression> expressions, final String description) {
+
+
+ return new ExpressionAdapter() {
+
+ private Collection<Object> col;
+
+ @Override
+ public Object evaluate(Exchange exchange) {
+ StringBuilder buffer = new StringBuilder();
+ for (Object obj : col) {
+ if (obj instanceof Expression) {
+ Expression expression = (Expression) obj;
String text = expression.evaluate(exchange, String.class);
if (text != null) {
buffer.append(text);
}
+ } else {
+ buffer.append((String) obj);
}
}
return buffer.toString();
}
@Override
public void init(CamelContext context) {
- boolean constant = false;
for (Expression expression : expressions) {
expression.init(context);
- constant |= expression instanceof ConstantExpressionAdapter;
}
- if (constant) {
- // okay some of the expressions are constant so we can optimize and avoid
- // evaluate them but use their constant value as-is directly
- // this can be common with the simple language where you use it for templating
- // by mixing string text and simple functions together (or via the log EIP)
- col = new ArrayList<>(expressions.size());
+
+ if(col == null) {
+ Collection<Object> preprocessedExpression = new ArrayList<>(expressions.size());
for (Expression expression : expressions) {
if (expression instanceof ConstantExpressionAdapter) {
+ expression.init(context);
Review Comment:
it's already initialized so it is not needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1469259099
PR 3.x: https://github.com/apache/camel/pull/9531
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1464857936
This PR causes NPE errors on main, so hold until we fixed this or we need to revert and drop this PRs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1427063886
Hi @essobedo reviewing the class, I agree with you. This class already knows if the expression is constant or not. Thus, for me you are right! Thanks a lot.
I'll write the unit test and use the approach you said, and we can discuss it again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] essobedo commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1461636243
@rhuan080 Would you mind creating a new PR for the branch `camel-3.x` for Camel 3.21?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1460055604
what is the status of this? And I think its safer to do this in 3.21 release only, and not 3.20.x to keep it as-is.
This is a corner case and simple language is used a lot in Camel routes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1435977431
I'm sorry for closing this issue. It was just a click error, and the button to close does not have a double check 🤦♂️
Thank you @essobedo and @oscerd !
As the @davsclaus that the init is just called at the Camel bootstrap and the init method is not called inside a parallel process, I think just preventing the collection from being re-initialized in case of calling the init again is sufficient. I will update the issue in Jira with these information soon..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] essobedo commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1426831598
> I have changed the synchronized block to lock. Now the evaluate method tries to get the ReadLock that does not block the thread for multiple ReadLock. The `init` method is using the WriteLock, thus the `evaluate` method is blocked only in case of a WriteLock on `init` method. It avoids the `evaluate` method to be blocked unnecessarily.
>
It if needs to be fixed, it is not the right way to fix it, you don't even need locks here. If you really want to fix it, you will have to rewrite the code of the `concatExpression`, to have 2 classes instead of one (for constants only and not only constants). The problem is with the case of the not only constants, for it, you simply need to set the content of `col` once for all while initializing the instance instead of doing it in the init method. Moreover, you would need to add a unit test to prove that the bug is really fixed, [see TDD](https://en.wikipedia.org/wiki/Test-driven_development).
> I think we should develop this method as thread-safe, as it is cached and can be used in a multithreaded task (like parallel or some internal multithread task). Thus, to avoid an issue now or in future, I think it should be immutable or thread-safe
This decision doesn't really belong to us, especially if we disagree, I guess that someone like @davsclaus or @oscerd could eventually decide. But, here, according to your stack trace, if we get this problem it is because the expressions are misused such that the threads spend their time to re-initialize the same expression over and over again instead of only once at startup as the framework does, I don't believe that for a production-ready code, we would really want this behavior, so it tends to prove that it is more a misuse than a bug but it is only my point of view, let's see what others think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] oscerd commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "oscerd (via GitHub)" <gi...@apache.org>.
oscerd commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1429157216
From what I read this is really a corner case. I'm not sure we should fix that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on a diff in pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on code in PR #9332:
URL: https://github.com/apache/camel/pull/9332#discussion_r1109831005
##########
core/camel-support/src/main/java/org/apache/camel/support/builder/ExpressionBuilder.java:
##########
@@ -1577,74 +1578,122 @@ public String toString() {
* Returns an expression which returns the string concatenation value of the various
* expressions
*
+ * @param context the Camel context
* @param expressions the expression to be concatenated dynamically
* @return an expression which when evaluated will return the concatenated values
*/
- public static Expression concatExpression(final Collection<Expression> expressions) {
- return concatExpression(expressions, null);
+ public static Expression concatExpression(final CamelContext context, final Collection<Expression> expressions) {
+ return concatExpression(context, expressions, null);
}
/**
* Returns an expression which returns the string concatenation value of the various
* expressions
*
+ * @param context the Camel context
* @param expressions the expression to be concatenated dynamically
* @param description the text description of the expression
* @return an expression which when evaluated will return the concatenated values
*/
- public static Expression concatExpression(final Collection<Expression> expressions, final String description) {
- return new ExpressionAdapter() {
+ public static Expression concatExpression(final CamelContext context, final Collection<Expression> expressions, final String description) {
+
+ for (Expression expression : expressions) {
+ if(expression instanceof ConstantExpressionAdapter){
+ return concatExpressionOptimized(context, expressions, description);
+ }
+ }
- private Collection<Object> col;
+ return concatExpression(expressions,description);
+ }
+
+ /**
+ * Returns an expression which returns the string concatenation value of the various
+ * expressions
+ *
+ * @param expressions the expression to be concatenated dynamically
+ * @param description the text description of the expression
+ * @return an expression which when evaluated will return the concatenated values
+ */
+ private static Expression concatExpression(final Collection<Expression> expressions, final String description) {
+ return new ExpressionAdapter() {
@Override
public Object evaluate(Exchange exchange) {
StringBuilder buffer = new StringBuilder();
- if (col != null) {
- // optimize for constant expressions so we can do this a bit faster
- for (Object obj : col) {
- if (obj instanceof Expression) {
- Expression expression = (Expression) obj;
- String text = expression.evaluate(exchange, String.class);
- if (text != null) {
- buffer.append(text);
- }
- } else {
- buffer.append((String) obj);
- }
+ for (Expression expression : expressions) {
+ String text = expression.evaluate(exchange, String.class);
+ if (text != null) {
+ buffer.append(text);
}
+ }
+ return buffer.toString();
+ }
+
+ @Override
+ public void init(CamelContext context) {
+ boolean constant = false;
+ for (Expression expression : expressions) {
+ expression.init(context);
+ }
+ }
+
+ @Override
+ public String toString() {
+ if (description != null) {
+ return description;
} else {
- for (Expression expression : expressions) {
+ return "concat(" + expressions + ")";
+ }
+ }
+ };
+ }
+
+ /**
+ * Returns an optimized expression which returns the string concatenation value of the various.
+ * expressions
+ *
+ * @param context the Camel context
+ * @param expressions the expression to be concatenated dynamically
+ * @param description the text description of the expression
+ * @return an expression which when evaluated will return the concatenated values
+ */
+ private static Expression concatExpressionOptimized(final CamelContext context, final Collection<Expression> expressions, final String description) {
+ Collection<Object> preprocessedExpression = new ArrayList<>(expressions.size());
+ for (Expression expression : expressions) {
+ if (expression instanceof ConstantExpressionAdapter) {
+ expression.init(context);
Review Comment:
Hi @essobedo, Looking at the `ConstantExpression`, I think that calling the `init` so early is not a problem. Waiting for the @oscerd to confirm it for us.
About the lock, the problem is the `evaluate` method being called concurrently with the `init`calling. Thus, I think the lock should be put at the `evaluate` method as well. However, we came back to the penalty we said before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1435080899
be careful to not overcomplicate this - the init of services in Camel are used during bootstrap and are called by Camel in sequence (no locking needed).
We should not do special weird code for one corner case with simple language that been in Camel since 1.x.
If you can come up with a better parser (then yeah) but do not sacrifice the init service lifecycle of Camel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1435478937
Hi @essobedo and @davsclaus. I fixed the init. I think just checking if the collection was initiated solves the question. Furthermore, the solution turns the code cleaner as now we have one implementation to optimized Expression and another one to unoptimized Expression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on a diff in pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on code in PR #9332:
URL: https://github.com/apache/camel/pull/9332#discussion_r1109050554
##########
core/camel-core/src/test/java/org/apache/camel/builder/ExpressionBuilderConcurrencyTest.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.builder;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.camel.ContextTestSupport;
+import org.apache.camel.Exchange;
+import org.apache.camel.Processor;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.model.language.SimpleExpression;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class ExpressionBuilderConcurrencyTest extends ContextTestSupport {
+
+ @Test
+ public void testConcatExpressionConcurrency() throws Exception {
+ MockEndpoint mockWithFailure = getMockEndpoint("mock:result");
+ mockWithFailure.expectedMessageCount(102);
+ mockWithFailure.assertIsSatisfied();
+ //assertMockEndpointsSatisfied();
+ List<Exchange> exchanges = mockWithFailure.getExchanges();
+ exchanges.stream()
+ .forEach(exchange -> Assertions.assertTrue(exchange.getMessage()
+ .getHeader("#CustomHeader", String.class)
+ .equals("This is a test a with startLabel: `Document` endLabel: `Document` and label: `ALabel`")));
+ }
+
+ @Override
+ protected RouteBuilder createRouteBuilder() throws Exception {
+
+ return new RouteBuilder() {
+ Map body = new HashMap() {
+ {
+ put("label", "ALabel");
+ put("startLabel", "Document");
+ put("endLabel", "Document");
+ }
+ };
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1430453156
Hi,
I added the unit test. Now, I'll rewrite the concatExpression to be immutable. @essobedo and @oscerd If you have questions or improvement about the unit test, please, tell me :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] essobedo commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1430486741
> The test provided uses components that are not available in this module. Thus, I created the test using what is available inside the camel-support module. I have reproduced the issue with my test.
You can put it in this package https://github.com/apache/camel/tree/main/core/camel-core/src/test/java/org/apache/camel/language
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 closed pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 closed pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
URL: https://github.com/apache/camel/pull/9332
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1464955525
Hi @davsclaus, ok! I'll send a PR to camel-3.x. I'll update the 3.x to add the commits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on a diff in pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on code in PR #9332:
URL: https://github.com/apache/camel/pull/9332#discussion_r1111121459
##########
core/camel-support/src/main/java/org/apache/camel/support/builder/ExpressionBuilder.java:
##########
@@ -1593,59 +1594,109 @@ public static Expression concatExpression(final Collection<Expression> expressio
* @return an expression which when evaluated will return the concatenated values
*/
public static Expression concatExpression(final Collection<Expression> expressions, final String description) {
- return new ExpressionAdapter() {
- private Collection<Object> col;
+ for (Expression expression : expressions) {
+ if(expression instanceof ConstantExpressionAdapter){
+ return concatExpressionOptimized(expressions, description);
+ }
+ }
+
+ return concatExpressionUnoptimized(expressions,description);
+ }
+
+ /**
+ * Returns an expression which returns the string concatenation value of the various
+ * expressions
+ *
+ * @param expressions the expression to be concatenated dynamically
+ * @param description the text description of the expression
+ * @return an expression which when evaluated will return the concatenated values
+ */
+ private static Expression concatExpressionUnoptimized(final Collection<Expression> expressions, final String description) {
+ return new ExpressionAdapter() {
@Override
public Object evaluate(Exchange exchange) {
StringBuilder buffer = new StringBuilder();
- if (col != null) {
- // optimize for constant expressions so we can do this a bit faster
- for (Object obj : col) {
- if (obj instanceof Expression) {
- Expression expression = (Expression) obj;
- String text = expression.evaluate(exchange, String.class);
- if (text != null) {
- buffer.append(text);
- }
- } else {
- buffer.append((String) obj);
- }
+ for (Expression expression : expressions) {
+ String text = expression.evaluate(exchange, String.class);
+ if (text != null) {
+ buffer.append(text);
}
+ }
+ return buffer.toString();
+ }
+
+ @Override
+ public void init(CamelContext context) {
+ for (Expression expression : expressions) {
+ expression.init(context);
+ }
+ }
+
+ @Override
+ public String toString() {
+ if (description != null) {
+ return description;
} else {
- for (Expression expression : expressions) {
+ return "concat(" + expressions + ")";
+ }
+ }
+ };
+ }
+
+ /**
+ * Returns an optimized expression which returns the string concatenation value of the various.
+ * expressions
+ *
+ * @param expressions the expression to be concatenated dynamically
+ * @param description the text description of the expression
+ * @return an expression which when evaluated will return the concatenated values
+ */
+ private static Expression concatExpressionOptimized(final Collection<Expression> expressions, final String description) {
+
+
+ return new ExpressionAdapter() {
+
+ private Collection<Object> col;
+
+ @Override
+ public Object evaluate(Exchange exchange) {
+ StringBuilder buffer = new StringBuilder();
+ for (Object obj : col) {
+ if (obj instanceof Expression) {
+ Expression expression = (Expression) obj;
String text = expression.evaluate(exchange, String.class);
if (text != null) {
buffer.append(text);
}
+ } else {
+ buffer.append((String) obj);
}
}
return buffer.toString();
}
@Override
public void init(CamelContext context) {
- boolean constant = false;
for (Expression expression : expressions) {
expression.init(context);
- constant |= expression instanceof ConstantExpressionAdapter;
}
- if (constant) {
- // okay some of the expressions are constant so we can optimize and avoid
- // evaluate them but use their constant value as-is directly
- // this can be common with the simple language where you use it for templating
- // by mixing string text and simple functions together (or via the log EIP)
- col = new ArrayList<>(expressions.size());
+
+ if(col == null) {
+ Collection<Object> preprocessedExpression = new ArrayList<>(expressions.size());
for (Expression expression : expressions) {
if (expression instanceof ConstantExpressionAdapter) {
+ expression.init(context);
Review Comment:
My fault. I'm sorry. Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1464859772
okay the optimized can be even better, if there are only ConstantExpressionAdapter then you can build the result string in init and return its constant value in evaluate
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] essobedo commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1426855865
> The best approach for me is to use an immutable object, however, if it is not possible I think we should consider using a lock or synchronized block.
>
Actually, immutability is the idea behind what I proposed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1426811738
Hi @essobedo, thank you for reviewing the PR.
I have changed the synchronized block to lock. Now the evaluate method tries to get the ReadLock that does not block the thread for multiple ReadLock. The `init` method is using the WriteLock, thus the `evaluate` method is blocked only in case of a WriteLock on `init` method. It avoids the `evaluate` method to be blocked unnecessarily.
I think we should develop this method as thread-safe, as it is cached and can be used in a multithreaded task (like parallel or some internal multithread task). Thus, to avoid an issue now or in future, I think it should be immutable or thread-safe
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] essobedo commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1429988834
@rhuan080 +1 I agree, if we can end up with a solution based on immutable classes, it is a good tradeoff
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] rhuan080 commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "rhuan080 (via GitHub)" <gi...@apache.org>.
rhuan080 commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1433575909
Hi @essobedo!
Added the test according to your suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] github-actions[bot] commented on pull request #9332: CAMEL-19014 - Fixing issue about concurrence on the SimpleLanguage
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9332:
URL: https://github.com/apache/camel/pull/9332#issuecomment-1426459482
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:warning: Please note that the changes on this PR may be **tested automatically**.
If necessary Apache Camel Committers may access logs and test results in the job summaries!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org