[GitHub] [openjpa] dazey3 commented on pull request #65: [OPENJPA-2817] dropping _transforming from PCClassFileTransformer and replace it by a minimal exclusion list

[GitBox <gi...@apache.org>]

dazey3 commented on pull request #65:
URL: https://github.com/apache/openjpa/pull/65#issuecomment-662738559


   Let's keep in mind that the issue we are trying to determine a fix for here is that `PCClassFileTransformer._transforming` does not support concurrent transformation access, correct?
   
   ### Current behavior:
   @struberg was correct in pointing out that the current, simple boolean implementation locks a Class transformer instance from being used by ANY Thread concurrently, including itself. All reentrant access to the same instance will just return NULL; no blocking, locking, or anything.
   
   In an environment where many Threads concurrently use the same Class transformer instance, a race condition occurs. Whichever Thread sets the boolean lock first, all other Threads seeking concurrent access return NULL and no transformation occurs. **This is the behavior that needs to change.** The boolean implementation needs to change behavior only to consider concurrent Thread access.
   
   ### Proposed fixes:
   It would appear that the reason for this current implementation was to prevent reentry for OpenJPA libraries/packages, and the implementation appears to do that. However, I think the original reason is irrelevant and should not be discussed in the fix. What is important is preserving the current behavior as much as possible. **The current implementation blocks ALL reentrance.**
   
   With the code change to a ThreadLocal boolean, the current behavior is preserved, but widened to support concurrent access. Rentrance would be allowed, but only on separate Threads. **I think the only question is: Is concurrent transformation thread safe (for instance, MetaDataRepository)?**
   
   The proposed code change to use an exclusion list feels like a re-implementation of the original fix that introduced the `PCClassFileTransformer._transforming` boolean. I think that is the incorrect path to take here because we are only guessing at the purpose of the original problem ( like what packages we need to prevent reentry for). What matters is what the code DOES right now and preserving that behavior, as much as possible, is the safest path


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org