Bug 331094

Comment 1 Tom Ware 2010-12-09 09:03:50 EST

Setting target and priority.  See the following page for the meanings of these fields:

http://wiki.eclipse.org/EclipseLink/Development/Bugs/Guidelines

Comment 2 Michael OBrien 2010-12-09 14:54:00 EST

>Sahoo, looks like something the next JPA 2.1 spec would be interested in reviewing.
I would forward more details on your use case, a possible reproduction stacktrace of a concurrent write contention or a read/write timing mismatch example and optionally a code diff of the spec class to the spec committee.

jpa-experts@sun.com

The suggestion that this static class be threadsafe may limit its usefullness - especially its' performance if it was synchronized rendering it effectively single-threaded.
In the javadoc for this spec class it is stated that implementations should be threadsafe - therefore it is up to the application(s) to handle their own concurrency issues, as the spec implementation should not be the synchronization bottleneck unless the potential for contention is very high.
I would expect that WeakHashMap was introduced by design so that the weak key could be GC'd easier - I think it is @NotThreadSafe by design.  This WeakHashMap inside the singleton inner class is marked as volatile - this looks to be sufficient.  There could be some debate on whether access to the singleton should be synchronized.

javax.persistence.spi.PersistenceProviderResolverHolder$DefaultPersistenceProviderResolver is part of the project javax.persistence 2.0.0 - its' source is maintained in the shared Eclipse Orbit repository. 

Thread [main] (Suspended)	
	Thread.getContextClassLoader() line: 1314 [local variables unavailable]	
	PersistenceProviderResolverHolder$DefaultPersistenceProviderResolver.getPersistenceProviders() line: not available [local variables unavailable]	
	Persistence.createEntityManagerFactory(String, Map) line: not available	
	Persistence.createEntityManagerFactory(String) line: not available

Comment 3 Michael OBrien 2010-12-09 15:33:41 EST

>Disregard my comment on contacting the JPA spec committee - unless it is about the interfaces themselves.
1) It turns out that we are responsible for the implementation of the javax.persistence spec classes and it's checkin to Orbit - especially this bootstrapping code  - I stand corrected
2) We need to verify your question about where the responsibility to be thread-safe lies during EMF creation/lookup?
a) With this JPA specification implementation in $DefaultPersistenceProviderResolver
or
b) with the JPA clients

It looks like "we" are resposible for thread safety for most of the functionality of the resolver and resolverHolder when getting the EMF - 2a)
I consulted the JPA 2.0 pdf (2009 Oct 08 version)
Specifically page 327 of section 9.3

"The PersistenceProviderResolverHolder class holds the PersistenceProviderResolver
instance that is in use. The implementation of PersistenceProviderResolver-
Holder must be threadsafe, but no guarantee is made against multiple threads setting the resolver."

and

"A PersistenceProviderResolver must be threadsafe"

>Therefore, yes, our implementation must be threadsafe.
specifically....
The following functions must be threadsafe
    public static PersistenceProviderResolver getPersistenceProviderResolver()
$DefaultPersistenceProviderResolver.clearCachedProviders()
$DefaultPersistenceProviderResolver.getPersistenceProviders()

The following functions are not required to be threadsafe
    // used by dynamic OSGI environments
    public static void setPersistenceProviderResolver(PersistenceProviderResolver resolver)

Note: The javadoc "Implementations must be thread-safe" - means Implementations of the JPA 2 spec - not implementations of JPA applications (from my previous comment)

I1) One issue may be dynamic OSGI environments where the list of providers may change at any time.  Therefore, the funtion aimed at OSGI environments "setPersistenceProviderResolver" may also need to be synchronized in that case.
I2) I don't think the @NotThreadSafe behavior of EntityManagers and PersistenceContexts applied here to the EMF.
Section 7.2 p.286
"An entity manager must not be shared among multiple concurrently executing threads, as the entitymanager and persistence context are not required to be threadsafe. Entity managers must only be accessed in a single-threaded manner."
- as P.287 clarifies this for us
"Methods of the EntityManagerFactory interface are threadsafe"

>Analysis and decision is pending
I think we need a patch for discussion
- thanks for raising this
/michael

Comment 4 Michael OBrien 2010-12-09 16:41:52 EST

>The following issues in bug# 238696 surrounding use and deprecation of the DCL pattern is relevant here
http://www.ibm.com/developerworks/java/library/j-dcl.html

Comment 5 Michael OBrien 2010-12-09 16:48:32 EST

>informally assigned this bug to investigate only (assinged to place in my queue but in new state) with no details on target/priority/status yet

Comment 6 Gordon Yorke 2011-01-19 11:00:12 EST

We should be careful with adding synchronization as this can have an impact on throughput as it serializes access to these artifacts.

the "singleton" field needs to be volatile but in the case of the "providers" map it would be better to have the threads build their own Map and set the map in the Resolver as a whole.  This way access to the Map is not synchronized and there is no corruption of the list by competing threads.  This may result in a thread over writing a previous threads work but that would not cause any problems as the Map is replaced as a whole and would only happen until the conflicting list building threads completed the build.  Subsequent threads would simply use the pre-built map.

Comment 7 Eclipse Webmaster 2022-06-09 10:26:02 EDT

The Eclipselink project has moved to Github: https://github.com/eclipse-ee4j/eclipselink