orpiske commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r997952712
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends
PollingConsumerSupport {
private static final Logger LOG =
LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
> I think we could ignore this issue as `volatile` here is used to avoiding
creating multi `ExecutorService` in `receive(long timeout)`.
I don't think volatile is enough to protect the code in this particular
scenario. A synchronized block / lock would be the correct to ensure this.
AFAIK, it only guarantees the visibility updates of itself (and other variables
declared around it) but cannot protect against concurrent writes of the
variable. And, since we have 2 operations happening on that variable in that
particular block (a fetch - from memory - and a write - to memory) it would
still be entirely possible for 2 threads to concurrently fetch a null value at
the same time.
```
// need to use a thread pool to perform the task so we can support timeout
if (executorService == null) {
executorService =
getEndpoint().getComponent().getOrCreatePollingConsumerExecutorService();
}
```
That said ... I have seen this pattern used elsewhere in the code and I
always wanted to fix it, but given a lot of other things we should take into
consideration before fixing it, I've always left it aside as suggested fix for
a major version.
So, TL,DR: IMHO, `volatile` is not OK, but we can leave it be for a larger
fix in the future.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
