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-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
