search

ops4j / org.ops4j.pax.transx

Transaction Manager and JMS / JDBC pooling support
Apache License 2.0
8 stars 7 forks source link

Unsynchronized multithread HashMap access in transaction manager wrappers #7

Closed vladimirfx closed 6 years ago

vladimirfx commented 6 years ago

As result many JVM freezes with stacks like this:

"schedulerSyncChanges-1" Id=162 in RUNNABLE at java.util.WeakHashMap.transfer(WeakHashMap.java:528) at java.util.WeakHashMap.resize(WeakHashMap.java:493) at java.util.WeakHashMap.put(WeakHashMap.java:466) at java.util.Map.computeIfAbsent(Map.java:958) at org.ops4j.pax.transx.tm.impl.AbstractTransactionManagerWrapper.getTransaction(AbstractTransactionManagerWrapper.java:49) at org.ops4j.pax.transx.connector.impl.GenericConnectionManager.getMci(GenericConnectionManager.java:186) at org.ops4j.pax.transx.connector.impl.GenericConnectionManager.allocateConnection(GenericConnectionManager.java:171) at org.ops4j.pax.transx.connector.impl.GenericConnectionManager.allocateConnection(GenericConnectionManager.java:167) at org.ops4j.pax.transx.jdbc.impl.TransxDataSource.getConnection(TransxDataSource.java:68) at org.ops4j.pax.transx.jdbc.impl.TransxDataSource.getConnection(TransxDataSource.java:59) at sun.reflect.GeneratedMethodAccessor328.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498)

graben commented 6 years ago

Can you provide more data to validate this issue. Better add a simple test case. Simply synchronizing access seems killing to me. Might be a future performance issue!

vladimirfx commented 6 years ago

I have similar stacktraces with frozen threads - nothing else. Reproducible test case for synchronization issues is nearly impossible thing, because hardware is involved (CPU L2/3 cache). Related discussion: https://stackoverflow.com/questions/38761222/hashmap-hang-in-concurrent-access

https://plumbr.io/blog/java/resizing-the-hashmap-dangers-ahead?utm_content=bufferf8e97&utm_medium=social&utm_source=twitter.com&utm_campaign=buffer

Performance hit there should be very low if measurable at all. On current JVM's synchronization itself has very low impact if threads do not clash when acquiring monitor. IMHO ConcurrentHashMap is overkill for this usecase.

graben commented 6 years ago

IMHO I only see need to synchronize the WeakMap not the other ones as handling with those objects should always be in the same Thread. Maybe @gnodet could approve this.

vladimirfx commented 6 years ago

In all TransactionManagerWrapper impls fields such resources and recoverables accessed and modified from:

  1. OSGI Config Admin threads when configurations for datasourses are un/registered.
  2. Any thread that install or uninstall, refresh configurer (pax-jdbc-config) bundle.
  3. OSGI service registration event threads when actual JDBC or JMS driver impl is registered.

Definitely it is not same thread.

graben commented 6 years ago

Close in favor of #10.