# HG changeset patch # User Asier Lostalé # Date 1375173404 -7200 # Node ID 3ba6ad3e165e27306eac1ddbd5d38febb48f47be # Parent 1cf0477d4c9de689772a3448313d1c6d7ef361a2 fixed bug 24421, fixed bug 24326: ADCachedStructures thread safe issues -getTab now completely initializes all tabs in the window -it is now synchronized Added test cases to check it diff -r 1cf0477d4c9d -r 3ba6ad3e165e modules/org.openbravo.client.application/src/org/openbravo/client/application/window/ApplicationDictionaryCachedStructures.java --- a/modules/org.openbravo.client.application/src/org/openbravo/client/application/window/ApplicationDictionaryCachedStructures.java Mon Jul 29 01:16:25 2013 +0200 +++ b/modules/org.openbravo.client.application/src/org/openbravo/client/application/window/ApplicationDictionaryCachedStructures.java Tue Jul 30 10:36:44 2013 +0200 @@ -11,7 +11,7 @@ * under the License. * The Original Code is Openbravo ERP. * The Initial Developer of the Original Code is Openbravo SLU - * All portions are Copyright (C) 2011 Openbravo SLU + * All portions are Copyright (C) 2011-2013 Openbravo SLU * All Rights Reserved. * Contributor(s): ______________________________________. ************************************************************************ @@ -40,6 +40,7 @@ import org.openbravo.model.ad.ui.AuxiliaryInput; import org.openbravo.model.ad.ui.Field; import org.openbravo.model.ad.ui.Tab; +import org.openbravo.model.ad.ui.Window; import org.openbravo.service.db.DalConnectionProvider; import org.openbravo.userinterface.selector.Selector; import org.openbravo.userinterface.selector.SelectorField; @@ -60,6 +61,7 @@ private Map> columnMap = new HashMap>(); private Map> auxInputMap = new HashMap>(); private Map comboTableDataMap = new HashMap(); + private List initializedWindows = new ArrayList(); private boolean useCache; @@ -70,18 +72,65 @@ useCache = indevelMods.list().size() == 0; } - public Tab getTab(String tabId) { + /** + * In case caching is enabled, Tab for tabId is returned from cache if present. If it is not, this + * tab and all the ones in the same window are initialized and cached. + *

+ * Note as this method is in charge of doing the full initialization, it should be invoked before + * any other getter in this class. Other case, partially initialized object could be cached, being + * potentially harmful if obtained from another thread and tried to be initialized. + * + * @param tabId + * , ID of the tab to look for + * @return Tab for the tabId, from cache if it is enabled + */ + public synchronized Tab getTab(String tabId) { if (useCache() && tabMap.containsKey(tabId)) { return tabMap.get(tabId); } Tab tab = OBDal.getInstance().get(Tab.class, tabId); + if (!useCache()) { + // not using cache, initialize just current tab and go + return tab; + } else { + // using cache, do complete initialization + initializeWindow(tab.getWindow().getId()); + } + return tab; + } + + /** + * Initialized all the tabs for a given window + */ + private void initializeWindow(String windowId) { + if (!useCache() || initializedWindows.contains(windowId)) { + return; + } + Window window = OBDal.getInstance().get(Window.class, windowId); + for (Tab tab : window.getADTabList()) { + initializeTab(tab); + } + initializedWindows.add(windowId); + } + + /** + * Initializes a tab and its related elements (table, fields, columns, auxiliary inputs and table + * combo data). If cache is enabled, tab is obtained from cache if it is already present and if + * not, it is put in cache after initialization + * + * @param tab + */ + private void initializeTab(Tab tab) { + String tabId = tab.getId(); initializeDALObject(tab); - initializeDALObject(tab.getADAuxiliaryInputList()); - initializeDALObject(tab.getADFieldList()); - initializeDALObject(tab.getTable()); - initializeDALObject(tab.getTable().getADColumnList()); - tabMap.put(tabId, tab); - return tab; + if (useCache()) { + tabMap.put(tabId, tab); + } + + // initialize other elements related with the tab + getAuxiliarInputList(tabId); + getFieldsOfTab(tabId); + getColumnsOfTable(tab.getTable().getId()); } public Table getTable(String tableId) { @@ -91,7 +140,9 @@ Table table = OBDal.getInstance().get(Table.class, tableId); initializeDALObject(table); initializeDALObject(table.getADColumnList()); - tableMap.put(tableId, table); + if (useCache()) { + tableMap.put(tableId, table); + } return table; } @@ -108,7 +159,9 @@ initializeDALObject(f.getColumn()); initializeColumn(f.getColumn()); } - fieldMap.put(tabId, fields); + if (useCache()) { + fieldMap.put(tabId, fields); + } return fields; } @@ -121,7 +174,9 @@ for (Column c : columns) { initializeColumn(c); } - columnMap.put(tableId, columns); + if (useCache()) { + columnMap.put(tableId, columns); + } return columns; } @@ -172,7 +227,9 @@ for (AuxiliaryInput auxIn : auxInputs) { initializeDALObject(auxIn); } - auxInputMap.put(tabId, auxInputs); + if (useCache()) { + auxInputMap.put(tabId, auxInputs); + } return auxInputs; } @@ -193,7 +250,7 @@ } catch (Exception e) { throw new OBException("Error while computing combo table data for column " + colName, e); } - if (comboTableData.canBeCached()) { + if (useCache() && comboTableData.canBeCached()) { comboTableDataMap.put(comboId, comboTableData); } return comboTableData; diff -r 1cf0477d4c9d -r 3ba6ad3e165e modules/org.openbravo.client.kernel/src/org/openbravo/client/kernel/KernelUtils.java --- a/modules/org.openbravo.client.kernel/src/org/openbravo/client/kernel/KernelUtils.java Mon Jul 29 01:16:25 2013 +0200 +++ b/modules/org.openbravo.client.kernel/src/org/openbravo/client/kernel/KernelUtils.java Tue Jul 30 10:36:44 2013 +0200 @@ -382,9 +382,13 @@ * * @param tab * The tab the object belongs to - * @return The parent tab of the given tab + * @return The parent tab of the given tab, null in case the tab is root */ public Tab getParentTab(Tab tab) { + if (tab.getTabLevel() == 0L) { + // root level, there is no parent + return null; + } List tabsOfWindow = tab.getWindow().getADTabList(); ArrayList entities = new ArrayList(); HashMap tabOfEntity = new HashMap(); diff -r 1cf0477d4c9d -r 3ba6ad3e165e src-test/org/openbravo/test/AllAntTaskTests.java --- a/src-test/org/openbravo/test/AllAntTaskTests.java Mon Jul 29 01:16:25 2013 +0200 +++ b/src-test/org/openbravo/test/AllAntTaskTests.java Tue Jul 30 10:36:44 2013 +0200 @@ -11,7 +11,7 @@ * under the License. * The Original Code is Openbravo ERP. * The Initial Developer of the Original Code is Openbravo SLU - * All portions are Copyright (C) 2009-2012 Openbravo SLU + * All portions are Copyright (C) 2009-2013 Openbravo SLU * All Rights Reserved. * Contributor(s): ______________________________________. ************************************************************************ @@ -23,6 +23,7 @@ import junit.framework.TestSuite; import org.openbravo.erpCommon.info.ClassicSelectorTest; +import org.openbravo.test.dal.ADCachedMultiThreadTest; import org.openbravo.test.dal.AdminContextTest; import org.openbravo.test.dal.DalComplexQueryRequisitionTest; import org.openbravo.test.dal.DalComplexQueryTestOrderLine; @@ -104,6 +105,7 @@ suite.addTestSuite(ReadByNameTest.class); suite.addTestSuite(AdminContextTest.class); suite.addTestSuite(ViewTest.class); + suite.addTestSuite(ADCachedMultiThreadTest.class); // expression suite.addTestSuite(EvaluationTest.class); diff -r 1cf0477d4c9d -r 3ba6ad3e165e src-test/org/openbravo/test/base/HiddenObjectHelper.java --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/src-test/org/openbravo/test/base/HiddenObjectHelper.java Tue Jul 30 10:36:44 2013 +0200 @@ -0,0 +1,86 @@ +/* + ************************************************************************* + * The contents of this file are subject to the Openbravo Public License + * Version 1.1 (the "License"), being the Mozilla Public License + * Version 1.1 with a permitted attribution clause; you may not use this + * file except in compliance with the License. You may obtain a copy of + * the License at http://www.openbravo.com/legal/license.html + * Software distributed under the License is distributed on an "AS IS" + * basis, WITHOUT WARRANTY OF ANY KIND, either express or implied. See the + * License for the specific language governing rights and limitations + * under the License. + * The Original Code is Openbravo ERP. + * The Initial Developer of the Original Code is Openbravo SLU + * All portions are Copyright (C) 2013 Openbravo SLU + * All Rights Reserved. + * Contributor(s): ______________________________________. + ************************************************************************ + */ + +package org.openbravo.test.base; + +import java.lang.reflect.Constructor; +import java.lang.reflect.Field; + +/** + * Utility class intended to be used in jUnit tests. + * + * It allows to read/modify non visible properties. + * + * @author alostale + * + */ +public class HiddenObjectHelper { + /** + * Initializes field in obj with its default constructor + * + */ + public static void initializeField(Object obj, String fieldName) throws Exception { + Field fld = getField(obj, fieldName); + + boolean originallyAccessible = fld.isAccessible(); + fld.setAccessible(true); + + Object o = get(obj, fieldName); + for (Constructor c : o.getClass().getConstructors()) { + if (c.getParameterTypes().length == 0) { + // default constructor found + o = c.newInstance(); + break; + } + } + + fld.setAccessible(originallyAccessible); + } + + /** + * Gets fieldName in obj + */ + public static Object get(Object obj, String fieldName) throws Exception { + Field fld = getField(obj, fieldName); + + boolean originallyAccessible = fld.isAccessible(); + fld.setAccessible(true); + + Object o = fld.get(obj); + fld.setAccessible(originallyAccessible); + return o; + } + + /** + * Sets value to obj.fieldName field + */ + public static void set(Object obj, String fieldName, Object value) throws Exception { + Field fld = getField(obj, fieldName); + boolean originallyAccessible = fld.isAccessible(); + fld.setAccessible(true); + fld.set(obj, value); + fld.setAccessible(originallyAccessible); + } + + private static Field getField(Object obj, String fieldName) throws Exception { + Class clazz = obj.getClass(); + return clazz.getDeclaredField(fieldName); + } + +} diff -r 1cf0477d4c9d -r 3ba6ad3e165e src-test/org/openbravo/test/dal/ADCachedMultiThreadTest.java --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/src-test/org/openbravo/test/dal/ADCachedMultiThreadTest.java Tue Jul 30 10:36:44 2013 +0200 @@ -0,0 +1,334 @@ +/* + ************************************************************************* + * The contents of this file are subject to the Openbravo Public License + * Version 1.1 (the "License"), being the Mozilla Public License + * Version 1.1 with a permitted attribution clause; you may not use this + * file except in compliance with the License. You may obtain a copy of + * the License at http://www.openbravo.com/legal/license.html + * Software distributed under the License is distributed on an "AS IS" + * basis, WITHOUT WARRANTY OF ANY KIND, either express or implied. See the + * License for the specific language governing rights and limitations + * under the License. + * The Original Code is Openbravo ERP. + * The Initial Developer of the Original Code is Openbravo SLU + * All portions are Copyright (C) 2013 Openbravo SLU + * All Rights Reserved. + * Contributor(s): ______________________________________. + ************************************************************************ + */ + +package org.openbravo.test.dal; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Random; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; + +import org.openbravo.client.application.window.ApplicationDictionaryCachedStructures; +import org.openbravo.client.kernel.KernelUtils; +import org.openbravo.dal.core.SessionHandler; +import org.openbravo.dal.service.OBDal; +import org.openbravo.test.base.BaseTest; +import org.openbravo.test.base.HiddenObjectHelper; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Test cases to verify multiple ApplicationDictionaryCachedStructures behavior when working + * concurrently with multiple threads. + * + * See issue https://issues.openbravo.com/view.php?id=24421 + * + * @author alostale + * + */ +public class ADCachedMultiThreadTest extends BaseTest { + private static final String BP_HEADER = "220"; + private static final String BP_CUSTOMER = "223"; + private static final int TEST_EXECUTIONS = 20; + private static final int MAX_DELAY = 250; + + private static Logger log = LoggerFactory.getLogger(ADCachedMultiThreadTest.class); + + /** + * This test executes using cache: + *

    + *
  • Thread 1 gets tab from ApplicationDictionaryCachedStructures and finishes + *
  • Thread 2 gets same tab and gets its parent + *
+ * + * Verifies tab object is completely initialized by Thread 1 so Thread 2 can work with it + * + */ + public void testParentTabCache() throws Exception { + executeTestParentTab(true); + } + + /** + * This test executes without using cache: + *
    + *
  • Thread 1 gets tab from ApplicationDictionaryCachedStructures and finishes + *
  • Thread 2 gets same tab and gets its parent + *
+ * + * Verifies tab object is completely initialized by Thread 1 so Thread 2 can work with it + * + */ + public void testParentTabNoCache() throws Exception { + executeTestParentTab(false); + } + + /** + * This test creates multiple threads invoking all public methods in + * ApplicationDictionaryCachedStructures for the same tab. + * + * Using cache + * + */ + public void testMultiCallsCache() throws Exception { + executeTestMultiCalls(true); + } + + /** + * This test creates multiple threads invoking all public methods in + * ApplicationDictionaryCachedStructures for the same tab. + * + * Not using cache + * + */ + public void testMultiCallsNoCache() throws Exception { + executeTestMultiCalls(false); + } + + private void executeTestParentTab(boolean cache) throws Exception { + log.info(" session id: {}", + Integer.toString(System.identityHashCode(OBDal.getInstance().getSession()))); + setSystemAdministratorContext(); + ApplicationDictionaryCachedStructures adcs = new ApplicationDictionaryCachedStructures(); + + // Force using cache even there are mods in dev + HiddenObjectHelper.set(adcs, "useCache", cache); + + ExecutorService executor = Executors.newFixedThreadPool(1); + + ArrayList> threads = new ArrayList>(); + threads.add(new TabLoader(adcs, BP_CUSTOMER, false)); + threads.add(new ParentLoader(adcs, BP_CUSTOMER, true)); + List> r = executor.invokeAll(threads, 5, TimeUnit.MINUTES); + ResultSummary summary = new ResultSummary(r); + summary.logResults(); + assertFalse("There are threads with errors", summary.hasErrors); + } + + private void executeTestMultiCalls(boolean cache) throws Exception { + long t0 = System.currentTimeMillis(); + setSystemAdministratorContext(); + + Map totalSummary = new HashMap(); + for (int i = 0; i < TEST_EXECUTIONS; i++) { + log.info("====== Starting execution #{} ============", i); + + long t = System.currentTimeMillis(); + + t = System.currentTimeMillis(); + ApplicationDictionaryCachedStructures adcs = new ApplicationDictionaryCachedStructures(); + // Force using cache even there are mods in dev + HiddenObjectHelper.set(adcs, "useCache", cache); + + ExecutorService executor = Executors.newFixedThreadPool(10); + + ArrayList threads = new ArrayList(); + threads.add(new TabLoader(adcs, BP_CUSTOMER, false)); + threads.add(new ParentLoader(adcs, BP_CUSTOMER, true)); + threads.add(new FieldListLoader(adcs, BP_CUSTOMER, true)); + threads.add(new ColumnsList(adcs, BP_CUSTOMER, true)); + threads.add(new AuxiliarInputList(adcs, BP_CUSTOMER, true)); + + threads.add(new TabLoader(adcs, BP_HEADER, false)); + threads.add(new ParentLoader(adcs, BP_HEADER, true)); + threads.add(new FieldListLoader(adcs, BP_HEADER, true)); + threads.add(new ColumnsList(adcs, BP_HEADER, true)); + threads.add(new AuxiliarInputList(adcs, BP_HEADER, true)); + + List> r = executor.invokeAll(threads, 5, TimeUnit.MINUTES); + log.info("All threads done in {} ms", System.currentTimeMillis() - t); + ResultSummary summary = new ResultSummary(r, totalSummary); + summary.logResults(); + + HiddenObjectHelper.initializeField(adcs, "tabMap"); + HiddenObjectHelper.initializeField(adcs, "tableMap"); + HiddenObjectHelper.initializeField(adcs, "fieldMap"); + HiddenObjectHelper.initializeField(adcs, "columnMap"); + HiddenObjectHelper.initializeField(adcs, "auxInputMap"); + HiddenObjectHelper.initializeField(adcs, "comboTableDataMap"); + // HiddenObjectHelper.initializeField(adcs, "initializedWindows"); + + log.info("====== Done execution #{} ============\n\n", i); + } + + log.info("Total Summary:"); + for (String key : totalSummary.keySet()) { + if (key.equals("Success")) { + log.info("{}\t: {}", key, totalSummary.get(key)); + } else { + log.error("{}\t: {}", key, totalSummary.get(key)); + } + } + + assertTrue("Threads with errors", totalSummary.size() == 1 + && totalSummary.get("Success") != null); + + log.info("Total time {}", System.currentTimeMillis() - t0); + } + + private abstract class Loader implements Callable { + protected ApplicationDictionaryCachedStructures cs; + protected String tabId; + protected boolean delay; + + Loader(ApplicationDictionaryCachedStructures cs, String tabId, boolean delay) { + this.cs = cs; + this.tabId = tabId; + this.delay = delay; + } + + protected abstract void doAction(); + + @Override + public Long call() { + setSystemAdministratorContext(); + long t = System.currentTimeMillis(); + log.info("Start thread {}", this.getClass().getName(), tabId); + delay(); + + doAction(); + + SessionHandler.getInstance().commitAndClose(); + OBDal.getInstance().getSession().disconnect(); + + log.info("thread {} done in {} ms", this.getClass().getName(), System.currentTimeMillis() - t); + return System.currentTimeMillis() - t; + } + + private void delay() { + if (delay) { + Random randomGenerator = new Random(); + int delayMs = randomGenerator.nextInt(MAX_DELAY); + log.info(" delaying thread {} ms", delayMs); + try { + Thread.sleep(delayMs); + } catch (InterruptedException e) { + + } + } + } + } + + private class TabLoader extends Loader { + TabLoader(ApplicationDictionaryCachedStructures cs, String tabId, boolean delay) { + super(cs, tabId, delay); + } + + @Override + protected void doAction() { + cs.getTab(tabId); + } + } + + private class ParentLoader extends Loader { + ParentLoader(ApplicationDictionaryCachedStructures cs, String tabId, boolean delay) { + super(cs, tabId, delay); + } + + @Override + protected void doAction() { + KernelUtils.getInstance().getParentTab(cs.getTab(tabId)); + } + } + + private class FieldListLoader extends Loader { + FieldListLoader(ApplicationDictionaryCachedStructures cs, String tabId, boolean delay) { + super(cs, tabId, delay); + } + + @Override + protected void doAction() { + cs.getFieldsOfTab(tabId); + } + } + + private class AuxiliarInputList extends Loader { + AuxiliarInputList(ApplicationDictionaryCachedStructures cs, String tabId, boolean delay) { + super(cs, tabId, delay); + } + + @Override + protected void doAction() { + cs.getAuxiliarInputList(tabId); + } + } + + private class ColumnsList extends Loader { + ColumnsList(ApplicationDictionaryCachedStructures cs, String tabId, boolean delay) { + super(cs, tabId, delay); + } + + @Override + protected void doAction() { + cs.getColumnsOfTable(cs.getTab(tabId).getTable().getId()); + } + } + + private class ResultSummary { + + private List> r; + private boolean hasErrors; + private Map totalSummary; + + public ResultSummary(List> r) { + this.r = r; + } + + public ResultSummary(List> r, Map totalSummary) { + this.r = r; + this.totalSummary = totalSummary; + } + + public void logResults() { + Map summary = new HashMap(); + for (Future f : r) { + try { + if (f.isCancelled()) { + addSummary(summary, totalSummary, "Cancelled"); + } else if (f.get() > 0) { + addSummary(summary, totalSummary, "Success"); + } + } catch (Throwable tr) { + addSummary(summary, totalSummary, tr.getMessage()); + } + } + for (String key : summary.keySet()) { + if (key.equals("Success")) { + log.info("{}\t: {}", key, summary.get(key)); + } else { + hasErrors = true; + log.error("{}\t: {}", key, summary.get(key)); + } + } + } + + private void addSummary(Map summary, Map totalS, String key) { + int total = (summary.get(key) == null ? 0 : summary.get(key)) + 1; + summary.put(key, total); + if (totalS != null) { + addSummary(totalS, null, key); + } + } + } +}