0037301: Java Heap Memory can rise on login if batch size is big

# HG changeset patch
# User Antonio Moreno <antonio.moreno@openbravo.com>
# Date 1510825214 -3600
#      Thu Nov 16 10:40:14 2017 +0100
# Node ID 24f70a7ca1428863c3e36d64703a9cc6c6278dd6
# Parent  2cea76ce07e3fc33355290a876b64dfd918a35fa
Fixed issue 37301. Avoid usage of intermediate StringWriters so that response doesn't take too much heap.
The main problem is that the two intermediate StringWriters accumulate the whole response before it is sent back to the client. This causes Java Heap memory problems in case the batching is set to a big amount, and concurrency is high.
The solution is to remove these intermediate StringWriters. We have verified that the messages are not lost even in the case of low level database problems, because the client correctly detects the problem, and doesn't remove the records, and instead tries to send them again in the next synchronization. However, we have added the exception fields in case the import entry fails to be written, because it makes sense to have them properly set.

diff -r 2cea76ce07e3 -r 24f70a7ca142 src/org/openbravo/mobile/core/process/MobileService.java
--- a/src/org/openbravo/mobile/core/process/MobileService.java	Mon Nov 13 11:17:07 2017 +0100
+++ b/src/org/openbravo/mobile/core/process/MobileService.java	Thu Nov 16 10:40:14 2017 +0100
@@ -9,7 +9,6 @@
 package org.openbravo.mobile.core.process;
 
 import java.io.IOException;
-import java.io.StringWriter;
 import java.io.Writer;
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
@@ -133,17 +132,12 @@
             // first write to a stringwriter
             // if an exception occurs then the exception can be written to a fresh response
             // writer
-            final Writer w = new StringWriter();
+            final Writer w = response.getWriter();
             w.write("{\"response\":{");
             new MobileServiceProcessor().execServiceName(w, pathparts[1], (JSONObject) jsonsent);
             w.write("}}");
             w.close();
 
-            // got here so probably fine to write to the response now
-            final Writer respWriter = response.getWriter();
-            respWriter.write(w.toString());
-            respWriter.close();
-
           } else {
             response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
             writeResult(response, JsonUtils.convertExceptionToJson(new InvalidRequestException(
diff -r 2cea76ce07e3 -r 24f70a7ca142 src/org/openbravo/mobile/core/process/MobileServiceProcessor.java
--- a/src/org/openbravo/mobile/core/process/MobileServiceProcessor.java	Mon Nov 13 11:17:07 2017 +0100
+++ b/src/org/openbravo/mobile/core/process/MobileServiceProcessor.java	Thu Nov 16 10:40:14 2017 +0100
@@ -9,7 +9,6 @@
 package org.openbravo.mobile.core.process;
 
 import java.io.IOException;
-import java.io.StringWriter;
 import java.io.Writer;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -68,32 +67,13 @@
   public void execServiceName(Writer w, String serviceName, JSONObject jsonsent) throws Exception {
     List<? extends JSONProcess> processes = getServiceClassInstances(serviceName);
 
-    // Create a new writer for each process. Only write on the given writer the last execution
-    // result.
-    Writer tmpwriter = null;
-    int i = 0;
     for (JSONProcess process : processes) {
-      i++;
-      tmpwriter = new StringWriter();
-      try {
-        execProcess(tmpwriter, process, jsonsent);
-      } catch (Exception e) {
-        if (processes.size() == i
-            || (process instanceof SecuredJSONProcess && !((SecuredJSONProcess) process)
-                .executeNextServiceClassOnFailure())) {
-          tmpwriter.close();
-          throw e;
-        }
-      }
-      tmpwriter.close();
+      execProcess(w, process, jsonsent);
       if (!(process instanceof SecuredJSONProcess && ((SecuredJSONProcess) process)
           .executeNextServiceClass())) {
         break;
       }
     }
-    if (tmpwriter != null) {
-      w.write(tmpwriter.toString());
-    }
   }
 
   /**
@@ -112,7 +92,12 @@
     if (process instanceof SecuredJSONProcess) {
       if (!synchronizedMode && !bypassImportEntry
           && process instanceof DataSynchronizationImportProcess) {
-        ((DataSynchronizationImportProcess) process).executeCreateImportEntry(w, jsonsent);
+        try {
+          ((DataSynchronizationImportProcess) process).executeCreateImportEntry(w, jsonsent);
+        } catch (Exception e) {
+          log.error(this.getClass().getName() + ": Error when creating ImportEntry: ", e);
+          JSONRowConverter.addJSONExceptionFields(w, e);
+        }
       } else {
         ((SecuredJSONProcess) process).secureExec(w, jsonsent);
       }

diff --git a/src-test/org/openbravo/test/mobile/core/utils/AllowedErrorsHelper.java b/src-test/org/openbravo/test/mobile/core/utils/AllowedErrorsHelper.java
--- a/src-test/org/openbravo/test/mobile/core/utils/AllowedErrorsHelper.java
+++ b/src-test/org/openbravo/test/mobile/core/utils/AllowedErrorsHelper.java
@@ -70,6 +70,7 @@
     testsAllowedToHaveTomcatErrors.add("I32443_LayawayStaleData");
     testsAllowedToHaveTomcatErrors.add("DuplicatedTicketAreSavedProperly");
     testsAllowedToHaveTomcatErrors.add(TestSynchronizedSaleWithError.class.getSimpleName());
+    testsAllowedToHaveTomcatErrors.add("I37301_ImportEntryError");
 
     // To handle this case:
     // WARN org.openbravo.dal.core.OBContext - The user 3073EDF96A3C42CC86C7069E379522D2 does not
@@ -231,6 +232,7 @@
     testsAllowedToHaveJavascriptErrors.add("I34421_VerifiedReturnsNotAllowSaleswithReturn");
     testsAllowedToHaveJavascriptErrors.add("I34586_CashupErrorLoginRestricted");
     testsAllowedToHaveJavascriptErrors.add("I35829_VerifyOrderLineNotTaxes");
+    testsAllowedToHaveJavascriptErrors.add("I37301_ImportEntryError");
 
     // illegal allowed errors. these errors have to be fixed:
     testsAllowedToHaveJavascriptErrors.add("TerminalChangeAllowed");
diff --git a/src-test/org/openbravo/test/mobile/retail/mobilecore/database/DatabaseHelperStatement.java b/src-test/org/openbravo/test/mobile/retail/mobilecore/database/DatabaseHelperStatement.java
new file mode 100644
--- /dev/null
+++ b/src-test/org/openbravo/test/mobile/retail/mobilecore/database/DatabaseHelperStatement.java
@@ -0,0 +1,74 @@
+/*
+ *************************************************************************
+ * The contents of this file are subject to the Openbravo  Public  License
+ * Version  1.0  (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 S.L.U.
+ * All portions are Copyright (C) 2017 Openbravo S.L.U.
+ * All Rights Reserved.
+ * Contributor(s):
+ ************************************************************************
+ *
+ * @author = mdj
+ *
+ */
+
+package org.openbravo.test.mobile.retail.mobilecore.database;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.sql.Statement;
+
+import org.openbravo.test.mobile.core.utils.TestLogger;
+import org.openbravo.test.mobile.retail.mobilecore.utils.RetailUtils;
+import org.openbravo.test.mobile.retail.pack.database.ServerType;
+
+public abstract class DatabaseHelperStatement {
+
+  public void execute(final String postgresSql, final String oracleSql) {
+    execute(postgresSql, oracleSql, ServerType.SINGLESERVER);
+  }
+
+  public void execute(final String postgresSql, final String oracleSql, final ServerType server) {
+    Statement statement = null;
+    Connection connection = null;
+    String sql = null;
+    try {
+      connection = ConnectionHelper.getConnection(server);
+      statement = connection.createStatement();
+      if (ConnectionHelper.isOracle() && oracleSql != null) {
+        sql = oracleSql;
+      } else {
+        sql = postgresSql;
+      }
+      statement.execute(sql);
+      TestLogger.getLogger().debug(String.format("Executing the statement: %s", sql));
+    } catch (final SQLException e) {
+      final String errorMessage = RetailUtils.getOBErrorMessage(e.getMessage());
+      final String longErrorMessage = String.format("Error when executing query:\n\"%s\"\n\n%s",
+          sql, errorMessage);
+      TestLogger.getLogger().error(longErrorMessage);
+      throw new RuntimeException(longErrorMessage, e);
+    } finally {
+      if (statement != null) {
+        try {
+          statement.close();
+        } catch (final SQLException e) { /* ignored */
+        }
+      }
+      if (connection != null) {
+        try {
+          connection.close();
+        } catch (final SQLException e) { /* ignored */
+        }
+      }
+    }
+  }
+}
diff --git a/src-test/org/openbravo/test/mobile/retail/mobilecore/javascript/SynchronizationHelper.java b/src-test/org/openbravo/test/mobile/retail/mobilecore/javascript/SynchronizationHelper.java
--- a/src-test/org/openbravo/test/mobile/retail/mobilecore/javascript/SynchronizationHelper.java
+++ b/src-test/org/openbravo/test/mobile/retail/mobilecore/javascript/SynchronizationHelper.java
@@ -82,6 +82,39 @@
     return true;
   }
 
+  public static boolean waitUntiTheApplicationHasModelNotSynchronized() {
+    // wait for the SynchronizationHelper to be available
+    new OBWait() {
+      @Override
+      protected boolean until(final int currentIteration) {
+        return (Boolean) SeleniumHelper
+            .executeScriptWithReturn("return (typeof(OB) === 'object' && typeof(OB.UTIL) === 'object' && typeof(OB.UTIL.SynchronizationHelper) === 'object');");
+      }
+
+      @Override
+      protected void timeout() {
+        throw new RuntimeException(
+            "OB namespace or OB.UTIL.SynchronizationHelper namespace not found");
+      }
+    };
+    new OBWait(OBConstants.SYNCHRONIZATIONHELPER_POLLING_INTERVAL, 200) {
+      @Override
+      protected boolean until(final int currentIteration) {
+        return (Boolean) SeleniumHelper.executeScriptWithReturn(String
+            .format("OB.UTIL.SynchronizationHelper.isModelsNotSynchronized()"));
+      }
+
+      @Override
+      protected void timeout() {
+        TestLogger.getLogger().error(
+            "SynchronizationHelper has not reached a 'ModelsNotSynchronized' state.");
+        throw new TimeoutException(
+            "SynchronizationHelper has not reached a 'ModelsNotSynchronized' state.");
+      }
+    };
+    return true;
+  }
+
   public static void waitUntilDataIsInTheServer() {
     waitUntilDataIsInTheServer(true);
   }
diff --git a/src-test/org/openbravo/test/mobile/retail/pack/selenium/terminals/WebPOSTerminalHelper.java b/src-test/org/openbravo/test/mobile/retail/pack/selenium/terminals/WebPOSTerminalHelper.java
--- a/src-test/org/openbravo/test/mobile/retail/pack/selenium/terminals/WebPOSTerminalHelper.java
+++ b/src-test/org/openbravo/test/mobile/retail/pack/selenium/terminals/WebPOSTerminalHelper.java
@@ -321,7 +321,7 @@
     SequentialTestInfo.setIsOnline();
   }
 
-  private void waitForOnline() {
+  protected void waitForOnline() {
     final int waitInterval = 1000;
     final int maxTries = 10;
     new OBWait(waitInterval, maxTries) {
diff --git a/src-test/org/openbravo/test/mobile/retail/pack/selenium/tests/receipts/I37301_ImportEntryError.java b/src-test/org/openbravo/test/mobile/retail/pack/selenium/tests/receipts/I37301_ImportEntryError.java
new file mode 100644
--- /dev/null
+++ b/src-test/org/openbravo/test/mobile/retail/pack/selenium/tests/receipts/I37301_ImportEntryError.java
@@ -0,0 +1,106 @@
+/*
+ *************************************************************************
+ * The contents of this file are subject to the Openbravo  Public  License
+ * Version  1.0  (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 S.L.U.
+ * All portions are Copyright (C) 2017 Openbravo S.L.U.
+ * All Rights Reserved.
+ * Contributor(s):  ______________________________________.
+ ************************************************************************
+ */
+
+package org.openbravo.test.mobile.retail.pack.selenium.tests.receipts;
+
+import java.sql.ResultSet;
+import java.sql.SQLException;
+
+import org.junit.Test;
+import org.openbravo.test.mobile.retail.mobilecore.annotations.TestClassAnnotations;
+import org.openbravo.test.mobile.retail.mobilecore.database.DatabaseHelperSelect;
+import org.openbravo.test.mobile.retail.mobilecore.database.DatabaseHelperStatement;
+import org.openbravo.test.mobile.retail.mobilecore.javascript.BackboneHelper;
+import org.openbravo.test.mobile.retail.mobilecore.javascript.SynchronizationHelper;
+import org.openbravo.test.mobile.retail.pack.selenium.TestIdPack;
+import org.openbravo.test.mobile.retail.pack.selenium.terminals.WebPOSTerminalHelper;
+
+@TestClassAnnotations(isHighVolumeCompatible = true)
+public class I37301_ImportEntryError extends WebPOSTerminalHelper {
+
+  @Test
+  public void test() {
+    try {
+      // Create a constraint to make the Import Entry fail
+      new DatabaseHelperStatement() {
+      }.execute(
+          "ALTER TABLE c_import_entry ADD CONSTRAINT c_import_entry_error CHECK (importstatus IN ('Processed'));",
+          null);
+
+      // Create an order
+      final String receiptNo1 = BackboneHelper.getDocumentNo();
+      tap(TestIdPack.BUTTON_SEARCH);
+      write(TestIdPack.FIELD_SEARCH_TEXT,
+          TestIdPack.BUTTON_SEARCHPRODUCT_AVALANCHETRANSCEIVER.getRowName());
+      tap(TestIdPack.BUTTON_SEARCH_EXECUTE);
+      tap(TestIdPack.BUTTON_SEARCHPRODUCT_AVALANCHETRANSCEIVER);
+      verify(TestIdPack.LABEL_TOTALTOPAY, "150.50");
+      tap(TestIdPack.BUTTON_PAY);
+      tap(TestIdPack.BUTTON_PAY_EXACT);
+      tap(TestIdPack.BUTTON_PAY_DONE);
+
+      // Ensure that the import entry failed and the message is still in messages table
+      SynchronizationHelper.waitUntiTheApplicationHasModelNotSynchronized();
+
+      // Remove the constraint
+      new DatabaseHelperStatement() {
+      }.execute("ALTER TABLE c_import_entry DROP CONSTRAINT c_import_entry_error;", null);
+
+      // Create another order
+      final String receiptNo2 = BackboneHelper.getDocumentNo();
+      tap(TestIdPack.BUTTON_SEARCH);
+      write(TestIdPack.FIELD_SEARCH_TEXT,
+          TestIdPack.BUTTON_SEARCHPRODUCT_AVALANCHETRANSCEIVER.getRowName());
+      tap(TestIdPack.BUTTON_SEARCH_EXECUTE);
+      tap(TestIdPack.BUTTON_SEARCHPRODUCT_AVALANCHETRANSCEIVER);
+      verify(TestIdPack.LABEL_TOTALTOPAY, "150.50");
+      tap(TestIdPack.BUTTON_PAY);
+      tap(TestIdPack.BUTTON_PAY_EXACT);
+      tap(TestIdPack.BUTTON_PAY_DONE);
+      tap(TestIdPack.BUTTON_MENU);
+      tap(TestIdPack.BUTTON_MENU_RETRYCONNECT);
+      waitForOnline();
+
+      // Check that both order are sychronized
+      final String sql1 = String.format(
+          "select co.c_order_id from c_order co where co.documentno = '%s'", receiptNo1);
+      new DatabaseHelperSelect() {
+        @Override
+        protected void yieldResultSet(final ResultSet rs) throws SQLException {
+
+        }
+      }.execute(sql1, 1);
+      final String sql2 = String.format(
+          "select co.c_order_id from c_order co where co.documentno = '%s'", receiptNo2);
+      new DatabaseHelperSelect() {
+        @Override
+        protected void yieldResultSet(final ResultSet rs) throws SQLException {
+
+        }
+      }.execute(sql2, 1);
+
+    } catch (final Throwable e) {
+      // Remove the constraint
+      new DatabaseHelperStatement() {
+      }.execute("ALTER TABLE c_import_entry DROP CONSTRAINT c_import_entry_error;", null);
+      e.printStackTrace(System.err);
+      throw new RuntimeException(e);
+    }
+  }
+}