Re: svn commit: r1758927 - in /ofbiz/trunk: applications/accounting/src/main/java/org/apache/ofbiz/accounting/thirdparty/gosoftware/ applications/content/src/main/java/org/apache/ofbiz/content/survey/ applications/marketing/src/main/java/org/apache/ofbiz/s...

[Jacopo Cappellato-5]

Jacques,

after a cursory review it seems to me that in your commit there are a few
issues; for example:
1) you are adding a close statement to code that already had the close
statement in the "finally" block; your modification actually introduces a
code pattern that is not correct (if an exception is thrown your close
statement are not executed)
2) I suspect that you are closing the socket connection too early
in PcChargeApi

I suggest to revert this commit, spend more time in reviewing and testing
this contribution before submitting it again.

Thanks,

Jacopo

On Fri, Sep 2, 2016 at 12:07 PM, <[hidden email]> wrote:

> Author: jleroux
> Date: Fri Sep  2 10:07:31 2016
> New Revision: 1758927
>
> URL: http://svn.apache.org/viewvc?rev=1758927&view=rev
> Log:
> Fixes a bunch of small leaks (closes missing, only warnings in Eclipse)
> By-product: automatically sort few imports
>
> Modified:
>     ofbiz/trunk/applications/accounting/src/main/java/org/
> apache/ofbiz/accounting/thirdparty/gosoftware/PcChargeApi.java
>     ofbiz/trunk/applications/content/src/main/java/org/
> apache/ofbiz/content/survey/PdfSurveyServices.java
>     ofbiz/trunk/applications/marketing/src/main/java/org/
> apache/ofbiz/sfa/vcard/VCard.java
>     ofbiz/trunk/framework/base/src/main/java/org/apache/
> ofbiz/base/util/Debug.java
>     ofbiz/trunk/framework/entity/src/main/java/org/apache/
> ofbiz/entity/jdbc/DatabaseUtil.java
>
> Modified: ofbiz/trunk/applications/accounting/src/main/java/org/
> apache/ofbiz/accounting/thirdparty/gosoftware/PcChargeApi.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/
> accounting/src/main/java/org/apache/ofbiz/accounting/
> thirdparty/gosoftware/PcChargeApi.java?rev=1758927&
> r1=1758926&r2=1758927&view=diff
> ============================================================
> ==================
> --- ofbiz/trunk/applications/accounting/src/main/java/org/
> apache/ofbiz/accounting/thirdparty/gosoftware/PcChargeApi.java (original)
> +++ ofbiz/trunk/applications/accounting/src/main/java/org/
> apache/ofbiz/accounting/thirdparty/gosoftware/PcChargeApi.java Fri Sep  2
> 10:07:31 2016
> @@ -18,18 +18,17 @@
>   ************************************************************
> *******************/
>  package org.apache.ofbiz.accounting.thirdparty.gosoftware;
>
> +import java.io.DataInputStream;
>  import java.io.IOException;
>  import java.io.PrintStream;
> -import java.io.DataInputStream;
>  import java.net.Socket;
>
>  import javax.xml.parsers.ParserConfigurationException;
>
> -import org.apache.ofbiz.base.util.UtilXml;
> -import org.apache.ofbiz.base.util.ObjectType;
> -import org.apache.ofbiz.base.util.GeneralException;
>  import org.apache.ofbiz.base.util.Debug;
> -
> +import org.apache.ofbiz.base.util.GeneralException;
> +import org.apache.ofbiz.base.util.ObjectType;
> +import org.apache.ofbiz.base.util.UtilXml;
>  import org.w3c.dom.Document;
>  import org.w3c.dom.Element;
>  import org.xml.sax.SAXException;
> @@ -189,6 +188,7 @@ public class PcChargeApi {
>              Socket sock = new Socket(host, port);
>              PrintStream ps = new PrintStream(sock.getOutputStream());
>              DataInputStream dis = new DataInputStream(sock.
> getInputStream());
> +            sock.close();
>              ps.print(this.toString());
>              ps.flush();
>
>
> Modified: ofbiz/trunk/applications/content/src/main/java/org/
> apache/ofbiz/content/survey/PdfSurveyServices.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/
> content/src/main/java/org/apache/ofbiz/content/survey/
> PdfSurveyServices.java?rev=1758927&r1=1758926&r2=1758927&view=diff
> ============================================================
> ==================
> --- ofbiz/trunk/applications/content/src/main/java/org/
> apache/ofbiz/content/survey/PdfSurveyServices.java (original)
> +++ ofbiz/trunk/applications/content/src/main/java/org/
> apache/ofbiz/content/survey/PdfSurveyServices.java Fri Sep  2 10:07:31
> 2016
> @@ -585,6 +585,7 @@ public class PdfSurveyServices {
>                      ByteArrayOutputStream baos = new
> ByteArrayOutputStream();
>                      while ((c = fis.read()) != -1) baos.write(c);
>                      inputByteBuffer = ByteBuffer.wrap(baos.
> toByteArray());
> +                    fis.close();
>                  } catch (FileNotFoundException e) {
>                      throw(new GeneralException(e.getMessage()));
>                  } catch (IOException e) {
>
> Modified: ofbiz/trunk/applications/marketing/src/main/java/org/
> apache/ofbiz/sfa/vcard/VCard.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/
> marketing/src/main/java/org/apache/ofbiz/sfa/vcard/VCard.
> java?rev=1758927&r1=1758926&r2=1758927&view=diff
> ============================================================
> ==================
> --- ofbiz/trunk/applications/marketing/src/main/java/org/
> apache/ofbiz/sfa/vcard/VCard.java (original)
> +++ ofbiz/trunk/applications/marketing/src/main/java/org/
> apache/ofbiz/sfa/vcard/VCard.java Fri Sep  2 10:07:31 2016
> @@ -31,16 +31,6 @@ import java.util.List;
>  import java.util.Locale;
>  import java.util.Map;
>
> -import ezvcard.Ezvcard;
> -import ezvcard.io.text.VCardReader;
> -import ezvcard.parameter.AddressType;
> -import ezvcard.parameter.TelephoneType;
> -import ezvcard.parameter.EmailType;
> -import ezvcard.property.Address;
> -import ezvcard.property.Email;
> -import ezvcard.property.FormattedName;
> -import ezvcard.property.StructuredName;
> -import ezvcard.property.Telephone;
>  import org.apache.ofbiz.base.util.Debug;
>  import org.apache.ofbiz.base.util.FileUtil;
>  import org.apache.ofbiz.base.util.StringUtil;
> @@ -62,6 +52,17 @@ import org.apache.ofbiz.service.GenericS
>  import org.apache.ofbiz.service.LocalDispatcher;
>  import org.apache.ofbiz.service.ServiceUtil;
>
> +import ezvcard.Ezvcard;
> +import ezvcard.io.text.VCardReader;
> +import ezvcard.parameter.AddressType;
> +import ezvcard.parameter.EmailType;
> +import ezvcard.parameter.TelephoneType;
> +import ezvcard.property.Address;
> +import ezvcard.property.Email;
> +import ezvcard.property.FormattedName;
> +import ezvcard.property.StructuredName;
> +import ezvcard.property.Telephone;
> +
>  public class VCard {
>      public static final String module = VCard.class.getName();
>      public static final String resourceError = "MarketingUiLabels";
> @@ -71,8 +72,9 @@ public class VCard {
>       * @param dctx
>       * @param context
>       * @return
> +     * @throws IOException
>       */
> -    public static Map<String, Object> importVCard(DispatchContext dctx,
> Map<String, ? extends Object> context) {
> +    public static Map<String, Object> importVCard(DispatchContext dctx,
> Map<String, ? extends Object> context) throws IOException {
>          LocalDispatcher dispatcher = dctx.getDispatcher();
>          Delegator delegator = dctx.getDelegator();
>          Locale locale = (Locale) context.get("locale");
> @@ -84,10 +86,10 @@ public class VCard {
>          boolean isGroup = false;
>          List<Map<String, String>> partiesCreated = new
> ArrayList<Map<String,String>>();
>          List<Map<String, String>> partiesExist = new
> ArrayList<Map<String,String>>();
> -        String partyName = "";
> +        String partyName = ""; // TODO this is not used yet
> +        VCardReader vCardReader = new VCardReader(in);
>
>          try {
> -            VCardReader vCardReader = new VCardReader(in);
>              ezvcard.VCard vcard = null;
>              while ((vcard = vCardReader.readNext()) != null) {
>
> @@ -162,6 +164,7 @@ public class VCard {
>                      } else {
>                          //TODO change uncorrect labellisation
>                          String emailFormatErrMsg =
> UtilProperties.getMessage(resourceError, "SfaImportVCardEmailFormatError",
> locale);
> +                        vCardReader.close();
>                          return ServiceUtil.returnError(structuredName.getGiven()
> + " " + structuredName.getFamily() + " has " + emailFormatErrMsg);
>                      }
>                  }
> @@ -215,12 +218,13 @@ public class VCard {
>                      resp = dispatcher.runSync("createPartyIdentification",
> createPartyIdentificationMap);
>                  }
>              }
> -            vCardReader.close();
>          } catch (IOException | GenericEntityException |
> GenericServiceException e) {
>              Debug.logError(e, module);
> +            vCardReader.close();
>              return ServiceUtil.returnError(UtilProperties.getMessage(
> resourceError,
>                      "SfaImportVCardError", UtilMisc.toMap("errorString",
> e.getMessage()), locale));
>          }
> +        vCardReader.close();
>          result.put("partiesCreated", partiesCreated);
>          result.put("partiesExist", partiesExist);
>          return result;
>
> Modified: ofbiz/trunk/framework/base/src/main/java/org/apache/
> ofbiz/base/util/Debug.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/
> src/main/java/org/apache/ofbiz/base/util/Debug.java?
> rev=1758927&r1=1758926&r2=1758927&view=diff
> ============================================================
> ==================
> --- ofbiz/trunk/framework/base/src/main/java/org/apache/ofbiz/base/util/Debug.java
> (original)
> +++ ofbiz/trunk/framework/base/src/main/java/org/apache/ofbiz/base/util/Debug.java
> Fri Sep  2 10:07:31 2016
> @@ -108,6 +108,7 @@ public final class Debug {
>                  Formatter formatter = new Formatter(sb);
>                  formatter.format(msg, params);
>                  msg = sb.toString();
> +                formatter.close();
>              }
>
>              // log
>
> Modified: ofbiz/trunk/framework/entity/src/main/java/org/apache/
> ofbiz/entity/jdbc/DatabaseUtil.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/
> src/main/java/org/apache/ofbiz/entity/jdbc/DatabaseUtil.java?rev=1758927&
> r1=1758926&r2=1758927&view=diff
> ============================================================
> ==================
> --- ofbiz/trunk/framework/entity/src/main/java/org/apache/
> ofbiz/entity/jdbc/DatabaseUtil.java (original)
> +++ ofbiz/trunk/framework/entity/src/main/java/org/apache/
> ofbiz/entity/jdbc/DatabaseUtil.java Fri Sep  2 10:07:31 2016
> @@ -1883,6 +1883,7 @@ public class DatabaseUtil {
>              try {
>                  stmt = connection.createStatement();
>                  stmt.executeUpdate(sql2);
> +                stmt.close();
>              } catch (SQLException e2) {
>                  // if this also fails report original error, not this
> error...
>                  return "SQL Exception while executing the following:\n" +
> sql + "\nError was: " + e.toString();
>
>
>

[Jacques Le Roux]

Thanks Jacopo,

Reverted at revision: 1758935, I'll check that!

Jacques


Le 02/09/2016 à 12:30, Jacopo Cappellato a écrit :

> Jacques,
>
> after a cursory review it seems to me that in your commit there are a few
> issues; for example:
> 1) you are adding a close statement to code that already had the close
> statement in the "finally" block; your modification actually introduces a
> code pattern that is not correct (if an exception is thrown your close
> statement are not executed)
> 2) I suspect that you are closing the socket connection too early
> in PcChargeApi
>
> I suggest to revert this commit, spend more time in reviewing and testing
> this contribution before submitting it again.
>
> Thanks,
>
> Jacopo
>
> On Fri, Sep 2, 2016 at 12:07 PM, <[hidden email]> wrote:
>
>> Author: jleroux
>> Date: Fri Sep  2 10:07:31 2016
>> New Revision: 1758927
>>
>> URL: http://svn.apache.org/viewvc?rev=1758927&view=rev
>> Log:
>> Fixes a bunch of small leaks (closes missing, only warnings in Eclipse)
>> By-product: automatically sort few imports
>>
>> Modified:
>>      ofbiz/trunk/applications/accounting/src/main/java/org/
>> apache/ofbiz/accounting/thirdparty/gosoftware/PcChargeApi.java
>>      ofbiz/trunk/applications/content/src/main/java/org/
>> apache/ofbiz/content/survey/PdfSurveyServices.java
>>      ofbiz/trunk/applications/marketing/src/main/java/org/
>> apache/ofbiz/sfa/vcard/VCard.java
>>      ofbiz/trunk/framework/base/src/main/java/org/apache/
>> ofbiz/base/util/Debug.java
>>      ofbiz/trunk/framework/entity/src/main/java/org/apache/
>> ofbiz/entity/jdbc/DatabaseUtil.java
>>
>> Modified: ofbiz/trunk/applications/accounting/src/main/java/org/
>> apache/ofbiz/accounting/thirdparty/gosoftware/PcChargeApi.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/
>> accounting/src/main/java/org/apache/ofbiz/accounting/
>> thirdparty/gosoftware/PcChargeApi.java?rev=1758927&
>> r1=1758926&r2=1758927&view=diff
>> ============================================================
>> ==================
>> --- ofbiz/trunk/applications/accounting/src/main/java/org/
>> apache/ofbiz/accounting/thirdparty/gosoftware/PcChargeApi.java (original)
>> +++ ofbiz/trunk/applications/accounting/src/main/java/org/
>> apache/ofbiz/accounting/thirdparty/gosoftware/PcChargeApi.java Fri Sep  2
>> 10:07:31 2016
>> @@ -18,18 +18,17 @@
>>    ************************************************************
>> *******************/
>>   package org.apache.ofbiz.accounting.thirdparty.gosoftware;
>>
>> +import java.io.DataInputStream;
>>   import java.io.IOException;
>>   import java.io.PrintStream;
>> -import java.io.DataInputStream;
>>   import java.net.Socket;
>>
>>   import javax.xml.parsers.ParserConfigurationException;
>>
>> -import org.apache.ofbiz.base.util.UtilXml;
>> -import org.apache.ofbiz.base.util.ObjectType;
>> -import org.apache.ofbiz.base.util.GeneralException;
>>   import org.apache.ofbiz.base.util.Debug;
>> -
>> +import org.apache.ofbiz.base.util.GeneralException;
>> +import org.apache.ofbiz.base.util.ObjectType;
>> +import org.apache.ofbiz.base.util.UtilXml;
>>   import org.w3c.dom.Document;
>>   import org.w3c.dom.Element;
>>   import org.xml.sax.SAXException;
>> @@ -189,6 +188,7 @@ public class PcChargeApi {
>>               Socket sock = new Socket(host, port);
>>               PrintStream ps = new PrintStream(sock.getOutputStream());
>>               DataInputStream dis = new DataInputStream(sock.
>> getInputStream());
>> +            sock.close();
>>               ps.print(this.toString());
>>               ps.flush();
>>
>>
>> Modified: ofbiz/trunk/applications/content/src/main/java/org/
>> apache/ofbiz/content/survey/PdfSurveyServices.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/
>> content/src/main/java/org/apache/ofbiz/content/survey/
>> PdfSurveyServices.java?rev=1758927&r1=1758926&r2=1758927&view=diff
>> ============================================================
>> ==================
>> --- ofbiz/trunk/applications/content/src/main/java/org/
>> apache/ofbiz/content/survey/PdfSurveyServices.java (original)
>> +++ ofbiz/trunk/applications/content/src/main/java/org/
>> apache/ofbiz/content/survey/PdfSurveyServices.java Fri Sep  2 10:07:31
>> 2016
>> @@ -585,6 +585,7 @@ public class PdfSurveyServices {
>>                       ByteArrayOutputStream baos = new
>> ByteArrayOutputStream();
>>                       while ((c = fis.read()) != -1) baos.write(c);
>>                       inputByteBuffer = ByteBuffer.wrap(baos.
>> toByteArray());
>> +                    fis.close();
>>                   } catch (FileNotFoundException e) {
>>                       throw(new GeneralException(e.getMessage()));
>>                   } catch (IOException e) {
>>
>> Modified: ofbiz/trunk/applications/marketing/src/main/java/org/
>> apache/ofbiz/sfa/vcard/VCard.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/
>> marketing/src/main/java/org/apache/ofbiz/sfa/vcard/VCard.
>> java?rev=1758927&r1=1758926&r2=1758927&view=diff
>> ============================================================
>> ==================
>> --- ofbiz/trunk/applications/marketing/src/main/java/org/
>> apache/ofbiz/sfa/vcard/VCard.java (original)
>> +++ ofbiz/trunk/applications/marketing/src/main/java/org/
>> apache/ofbiz/sfa/vcard/VCard.java Fri Sep  2 10:07:31 2016
>> @@ -31,16 +31,6 @@ import java.util.List;
>>   import java.util.Locale;
>>   import java.util.Map;
>>
>> -import ezvcard.Ezvcard;
>> -import ezvcard.io.text.VCardReader;
>> -import ezvcard.parameter.AddressType;
>> -import ezvcard.parameter.TelephoneType;
>> -import ezvcard.parameter.EmailType;
>> -import ezvcard.property.Address;
>> -import ezvcard.property.Email;
>> -import ezvcard.property.FormattedName;
>> -import ezvcard.property.StructuredName;
>> -import ezvcard.property.Telephone;
>>   import org.apache.ofbiz.base.util.Debug;
>>   import org.apache.ofbiz.base.util.FileUtil;
>>   import org.apache.ofbiz.base.util.StringUtil;
>> @@ -62,6 +52,17 @@ import org.apache.ofbiz.service.GenericS
>>   import org.apache.ofbiz.service.LocalDispatcher;
>>   import org.apache.ofbiz.service.ServiceUtil;
>>
>> +import ezvcard.Ezvcard;
>> +import ezvcard.io.text.VCardReader;
>> +import ezvcard.parameter.AddressType;
>> +import ezvcard.parameter.EmailType;
>> +import ezvcard.parameter.TelephoneType;
>> +import ezvcard.property.Address;
>> +import ezvcard.property.Email;
>> +import ezvcard.property.FormattedName;
>> +import ezvcard.property.StructuredName;
>> +import ezvcard.property.Telephone;
>> +
>>   public class VCard {
>>       public static final String module = VCard.class.getName();
>>       public static final String resourceError = "MarketingUiLabels";
>> @@ -71,8 +72,9 @@ public class VCard {
>>        * @param dctx
>>        * @param context
>>        * @return
>> +     * @throws IOException
>>        */
>> -    public static Map<String, Object> importVCard(DispatchContext dctx,
>> Map<String, ? extends Object> context) {
>> +    public static Map<String, Object> importVCard(DispatchContext dctx,
>> Map<String, ? extends Object> context) throws IOException {
>>           LocalDispatcher dispatcher = dctx.getDispatcher();
>>           Delegator delegator = dctx.getDelegator();
>>           Locale locale = (Locale) context.get("locale");
>> @@ -84,10 +86,10 @@ public class VCard {
>>           boolean isGroup = false;
>>           List<Map<String, String>> partiesCreated = new
>> ArrayList<Map<String,String>>();
>>           List<Map<String, String>> partiesExist = new
>> ArrayList<Map<String,String>>();
>> -        String partyName = "";
>> +        String partyName = ""; // TODO this is not used yet
>> +        VCardReader vCardReader = new VCardReader(in);
>>
>>           try {
>> -            VCardReader vCardReader = new VCardReader(in);
>>               ezvcard.VCard vcard = null;
>>               while ((vcard = vCardReader.readNext()) != null) {
>>
>> @@ -162,6 +164,7 @@ public class VCard {
>>                       } else {
>>                           //TODO change uncorrect labellisation
>>                           String emailFormatErrMsg =
>> UtilProperties.getMessage(resourceError, "SfaImportVCardEmailFormatError",
>> locale);
>> +                        vCardReader.close();
>>                           return ServiceUtil.returnError(structuredName.getGiven()
>> + " " + structuredName.getFamily() + " has " + emailFormatErrMsg);
>>                       }
>>                   }
>> @@ -215,12 +218,13 @@ public class VCard {
>>                       resp = dispatcher.runSync("createPartyIdentification",
>> createPartyIdentificationMap);
>>                   }
>>               }
>> -            vCardReader.close();
>>           } catch (IOException | GenericEntityException |
>> GenericServiceException e) {
>>               Debug.logError(e, module);
>> +            vCardReader.close();
>>               return ServiceUtil.returnError(UtilProperties.getMessage(
>> resourceError,
>>                       "SfaImportVCardError", UtilMisc.toMap("errorString",
>> e.getMessage()), locale));
>>           }
>> +        vCardReader.close();
>>           result.put("partiesCreated", partiesCreated);
>>           result.put("partiesExist", partiesExist);
>>           return result;
>>
>> Modified: ofbiz/trunk/framework/base/src/main/java/org/apache/
>> ofbiz/base/util/Debug.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/
>> src/main/java/org/apache/ofbiz/base/util/Debug.java?
>> rev=1758927&r1=1758926&r2=1758927&view=diff
>> ============================================================
>> ==================
>> --- ofbiz/trunk/framework/base/src/main/java/org/apache/ofbiz/base/util/Debug.java
>> (original)
>> +++ ofbiz/trunk/framework/base/src/main/java/org/apache/ofbiz/base/util/Debug.java
>> Fri Sep  2 10:07:31 2016
>> @@ -108,6 +108,7 @@ public final class Debug {
>>                   Formatter formatter = new Formatter(sb);
>>                   formatter.format(msg, params);
>>                   msg = sb.toString();
>> +                formatter.close();
>>               }
>>
>>               // log
>>
>> Modified: ofbiz/trunk/framework/entity/src/main/java/org/apache/
>> ofbiz/entity/jdbc/DatabaseUtil.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/
>> src/main/java/org/apache/ofbiz/entity/jdbc/DatabaseUtil.java?rev=1758927&
>> r1=1758926&r2=1758927&view=diff
>> ============================================================
>> ==================
>> --- ofbiz/trunk/framework/entity/src/main/java/org/apache/
>> ofbiz/entity/jdbc/DatabaseUtil.java (original)
>> +++ ofbiz/trunk/framework/entity/src/main/java/org/apache/
>> ofbiz/entity/jdbc/DatabaseUtil.java Fri Sep  2 10:07:31 2016
>> @@ -1883,6 +1883,7 @@ public class DatabaseUtil {
>>               try {
>>                   stmt = connection.createStatement();
>>                   stmt.executeUpdate(sql2);
>> +                stmt.close();
>>               } catch (SQLException e2) {
>>                   // if this also fails report original error, not this
>> error...
>>                   return "SQL Exception while executing the following:\n" +
>> sql + "\nError was: " + e.toString();
>>
>>
>>

[Jacques Le Roux]

Le 02/09/2016 à 12:30, Jacopo Cappellato a écrit :
> 1) you are adding a close statement to code that already had the close
> statement in the "finally" block; your modification actually introduces a
> code pattern that is not correct (if an exception is thrown your close
> statement are not executed)

I guess you spoke about DatabaseUtil.java
The best option when closing resources (which implement|AutoCloseable) |is actually to use the try-with-resources statement. At least when there are
no performance issues.
I have fixed the issue you reported (was only in DatabaseUtil.java) this way because

  *   there are not performance issues, we anyway create a Statement in both cases, we don't reuse one.
  * the finally part is cleaner

I have also used the try-with-resources statement in other places where it fitted.

There is only 1 place where I was undecided: ScriptUtil.parseScript() which uses GroovyUtil.parseClass().
I put only a logError() there and returned null.
I did not throw an IOException because else it spreads everywhere in FlexibleStringExpander.
Here introducing Optional would be better, but I have no time or that today.

> 2) I suspect that you are closing the socket connection too early
> in PcChargeApi

I see no problem with this point, the "sock" socket connection is not used in code below.

I have created https://issues.apache.org/jira/browse/OFBIZ-8115 for that and committed my work at

I'll continue to carefully clean stuff Eclipse reports

Jacques

[Jacopo Cappellato-5]

On Fri, Sep 2, 2016 at 3:19 PM, Jacques Le Roux <
[hidden email]> wrote:

> Le 02/09/2016 à 12:30, Jacopo Cappellato a écrit :
>
>> ...
>
> 2) I suspect that you are closing the socket connection too early
>> in PcChargeApi
>>
>
> I see no problem with this point, the "sock" socket connection is not used
> in code below.
> ...
> Jacques
>
>

Jacques,

at the moment I don't have time to explain but after your response above I
am a bit concerned about the effort that you are carrying on, because you
may break other code without even suspecting it.
I have to go now, if others can explain then great, otherwise I will follow
up with you asap.

Jacopo

[Jacques Le Roux]

Le 02/09/2016 à 18:46, Jacopo Cappellato a écrit :

> On Fri, Sep 2, 2016 at 3:19 PM, Jacques Le Roux <
> [hidden email]> wrote:
>
>> Le 02/09/2016 à 12:30, Jacopo Cappellato a écrit :
>>
>>> ...
>> 2) I suspect that you are closing the socket connection too early
>>> in PcChargeApi
>>>
>> I see no problem with this point, the "sock" socket connection is not used
>> in code below.
>> ...
>> Jacques
>>
>>
> Jacques,
>
> at the moment I don't have time to explain but after your response above I
> am a bit concerned about the effort that you are carrying on, because you
> may break other code without even suspecting it.
> I have to go now, if others can explain then great, otherwise I will follow
> up with you asap.
>
> Jacopo
>

Ah I think I got your point when back, I should better close the Socket later, done at r1758990

Jacques

[taher]

Jacques this is still problematic, you are closing the streams upon
exceptions! now all kinds of memory leaks and side effects could take place
because you are not ensuring closing all streams properly.

This needs a revert and a full review to make things tight in terms of
memory.

On Fri, Sep 2, 2016 at 8:08 PM, Jacques Le Roux <
[hidden email]> wrote:

> Le 02/09/2016 à 18:46, Jacopo Cappellato a écrit :
>
>> On Fri, Sep 2, 2016 at 3:19 PM, Jacques Le Roux <
>> [hidden email]> wrote:
>>
>> Le 02/09/2016 à 12:30, Jacopo Cappellato a écrit :
>>>
>>> ...
>>>>
>>> 2) I suspect that you are closing the socket connection too early
>>>
>>>> in PcChargeApi
>>>>
>>>> I see no problem with this point, the "sock" socket connection is not
>>> used
>>> in code below.
>>> ...
>>> Jacques
>>>
>>>
>>> Jacques,
>>
>> at the moment I don't have time to explain but after your response above I
>> am a bit concerned about the effort that you are carrying on, because you
>> may break other code without even suspecting it.
>> I have to go now, if others can explain then great, otherwise I will
>> follow
>> up with you asap.
>>
>> Jacopo
>>
>> Ah I think I got your point when back, I should better close the Socket
> later, done at r1758990
>
> Jacques
>
>

[Jacques Le Roux]

Why should I not close the Socket once the DataInputStream derived from it has been used?

In the case of an exception occurs will not the resource be leaked?

Do you mean that I should also close the DataInputStream before closing the Socket?

ie something like

Index: applications/accounting/src/main/java/org/apache/ofbiz/accounting/thirdparty/gosoftware/PcChargeApi.java
===================================================================
--- applications/accounting/src/main/java/org/apache/ofbiz/accounting/thirdparty/gosoftware/PcChargeApi.java (revision 1758990)
+++ applications/accounting/src/main/java/org/apache/ofbiz/accounting/thirdparty/gosoftware/PcChargeApi.java (working copy)
@@ -200,14 +200,17 @@
              try {
                  outDoc = UtilXml.readXmlDocument(buf.toString(), false);
              } catch (ParserConfigurationException e) {
+                dis.close();
                  sock.close();
                  throw new GeneralException(e);
              } catch (SAXException e) {
+                dis.close();
                  sock.close();
                  throw new GeneralException(e);
              }

              PcChargeApi out = new PcChargeApi(outDoc);
+            dis.close();
              sock.close();
              return out;
          } else {

Jacques


Le 02/09/2016 à 19:16, Taher Alkhateeb a écrit :

> Jacques this is still problematic, you are closing the streams upon
> exceptions! now all kinds of memory leaks and side effects could take place
> because you are not ensuring closing all streams properly.
>
> This needs a revert and a full review to make things tight in terms of
> memory.
>
> On Fri, Sep 2, 2016 at 8:08 PM, Jacques Le Roux <
> [hidden email]> wrote:
>
>> Le 02/09/2016 à 18:46, Jacopo Cappellato a écrit :
>>
>>> On Fri, Sep 2, 2016 at 3:19 PM, Jacques Le Roux <
>>> [hidden email]> wrote:
>>>
>>> Le 02/09/2016 à 12:30, Jacopo Cappellato a écrit :
>>>> ...
>>>> 2) I suspect that you are closing the socket connection too early
>>>>
>>>>> in PcChargeApi
>>>>>
>>>>> I see no problem with this point, the "sock" socket connection is not
>>>> used
>>>> in code below.
>>>> ...
>>>> Jacques
>>>>
>>>>
>>>> Jacques,
>>> at the moment I don't have time to explain but after your response above I
>>> am a bit concerned about the effort that you are carrying on, because you
>>> may break other code without even suspecting it.
>>> I have to go now, if others can explain then great, otherwise I will
>>> follow
>>> up with you asap.
>>>
>>> Jacopo
>>>
>>> Ah I think I got your point when back, I should better close the Socket
>> later, done at r1758990
>>
>> Jacques
>>
>>

[taher]

Hi Jacques,

The resources are not properly closed in your commit. It would take time to
explain, suffice to say that your code leads to possible memory leaks. That
is why I suggest a full review and carefully applying the stream memory
management. There are multiple ways to fix this, some related to Java 7,
but it would take time to explain, and it is your initiative so I leave it
up to you to fix it.

Regards,

Taher Alkhateeb

On Fri, Sep 2, 2016 at 8:37 PM, Jacques Le Roux <
[hidden email]> wrote:

> Why should I not close the Socket once the DataInputStream derived from it
> has been used?
>
> In the case of an exception occurs will not the resource be leaked?
>
> Do you mean that I should also close the DataInputStream before closing
> the Socket?
>
> ie something like
>
> Index: applications/accounting/src/main/java/org/apache/ofbiz/accou
> nting/thirdparty/gosoftware/PcChargeApi.java
> ===================================================================
> --- applications/accounting/src/main/java/org/apache/ofbiz/accou
> nting/thirdparty/gosoftware/PcChargeApi.java (revision 1758990)
> +++ applications/accounting/src/main/java/org/apache/ofbiz/accou
> nting/thirdparty/gosoftware/PcChargeApi.java (working copy)
> @@ -200,14 +200,17 @@
>              try {
>                  outDoc = UtilXml.readXmlDocument(buf.toString(), false);
>              } catch (ParserConfigurationException e) {
> +                dis.close();
>                  sock.close();
>                  throw new GeneralException(e);
>              } catch (SAXException e) {
> +                dis.close();
>                  sock.close();
>                  throw new GeneralException(e);
>              }
>
>              PcChargeApi out = new PcChargeApi(outDoc);
> +            dis.close();
>              sock.close();
>              return out;
>          } else {
>
> Jacques
>
>
>
> Le 02/09/2016 à 19:16, Taher Alkhateeb a écrit :
>
>> Jacques this is still problematic, you are closing the streams upon
>> exceptions! now all kinds of memory leaks and side effects could take
>> place
>> because you are not ensuring closing all streams properly.
>>
>> This needs a revert and a full review to make things tight in terms of
>> memory.
>>
>> On Fri, Sep 2, 2016 at 8:08 PM, Jacques Le Roux <
>> [hidden email]> wrote:
>>
>> Le 02/09/2016 à 18:46, Jacopo Cappellato a écrit :
>>>
>>> On Fri, Sep 2, 2016 at 3:19 PM, Jacques Le Roux <
>>>> [hidden email]> wrote:
>>>>
>>>> Le 02/09/2016 à 12:30, Jacopo Cappellato a écrit :
>>>>
>>>>> ...
>>>>> 2) I suspect that you are closing the socket connection too early
>>>>>
>>>>> in PcChargeApi
>>>>>>
>>>>>> I see no problem with this point, the "sock" socket connection is not
>>>>>>
>>>>> used
>>>>> in code below.
>>>>> ...
>>>>> Jacques
>>>>>
>>>>>
>>>>> Jacques,
>>>>>
>>>> at the moment I don't have time to explain but after your response
>>>> above I
>>>> am a bit concerned about the effort that you are carrying on, because
>>>> you
>>>> may break other code without even suspecting it.
>>>> I have to go now, if others can explain then great, otherwise I will
>>>> follow
>>>> up with you asap.
>>>>
>>>> Jacopo
>>>>
>>>> Ah I think I got your point when back, I should better close the Socket
>>>>
>>> later, done at r1758990
>>>
>>> Jacques
>>>
>>>
>>>
>

[Jacques Le Roux]

Hi Taher, Jacopo,

Sincerely reading https://docs.oracle.com/javase/8/docs/api/java/net/Socket.html#close--

/<<public void close()
            throws IOException
Closes this socket.
Any thread currently blocked in an I/O operation upon this socket will throw a SocketException.
Once a socket has been closed, it is not available for further networking use (i.e. can't be reconnected or rebound). A new socket needs to be created.
Closing this socket will also close the socket's InputStream and OutputStream.
If this socket has an associated channel then the channel is closed as well.>>
/

the only problem I can see is if several threads would simultaneously access the same PcChargeApi object and its inner Socket has been closed then a
SocketException will be fired./
/

But in the PcChargeServices class, each call to PcChargeApi.send() is done after the creation of a local PcChargeApi object through
PcChargeApi.getApi(). So I can't see how this object could be shared.

Hence closing the socket, as I did in my last commit, should not "leads to possible memory leaks". By closing it I close also the socket's InputStream
so no leaks should be expected.

BTW you may wonder (I did!), because PcChargeApi.getApi() returns an escaped PcChargeApi object. So it's not thread safe, and another thread could
access it concurrently (being a PcChargeServices.ccAuth() or any other of the PcChargeServices.cc*() public methods)

And if several threads simultaneously access the same PcChargeApi object and its Socket has been closed, then a SocketException would be fired.

But, because the Socket does not escape from the PcChargeApi.send(), the Socket is thread safe and can't be shared between threads (even if the
PcChargeApi object can be) and I don't see how a SocketException could be fired///
/

So I see no problem with my currently committed solution.  I initially missed to close the Socket in the catched exceptions which is why, I believe,
Jacopo was concerned.

If I miss something please explain

Note that in the original code the socket was not closed, so a resource leak existed.
It's not a big deal if you don't use a lot of calls to PcChargeApi.send() during the JVM session, since the JVM will clear it when finished.
But it seems to me that it's better to avoid crossing this issue.

And last but not least, this is in a peripheral thirdparty\gosoftware, so there is currently nothing to urgently worry about in trunk. We can exchange
and I'd be happy to stand corrected if I get a clear answer.

Jacques


Le 02/09/2016 à 20:43, Taher Alkhateeb a écrit :

> Hi Jacques,
>
> The resources are not properly closed in your commit. It would take time to
> explain, suffice to say that your code leads to possible memory leaks. That
> is why I suggest a full review and carefully applying the stream memory
> management. There are multiple ways to fix this, some related to Java 7,
> but it would take time to explain, and it is your initiative so I leave it
> up to you to fix it.
>
> Regards,
>
> Taher Alkhateeb
>
> On Fri, Sep 2, 2016 at 8:37 PM, Jacques Le Roux <
> [hidden email]> wrote:
>
>> Why should I not close the Socket once the DataInputStream derived from it
>> has been used?
>>
>> In the case of an exception occurs will not the resource be leaked?
>>
>> Do you mean that I should also close the DataInputStream before closing
>> the Socket?
>>
>> ie something like
>>
>> Index: applications/accounting/src/main/java/org/apache/ofbiz/accou
>> nting/thirdparty/gosoftware/PcChargeApi.java
>> ===================================================================
>> --- applications/accounting/src/main/java/org/apache/ofbiz/accou
>> nting/thirdparty/gosoftware/PcChargeApi.java (revision 1758990)
>> +++ applications/accounting/src/main/java/org/apache/ofbiz/accou
>> nting/thirdparty/gosoftware/PcChargeApi.java (working copy)
>> @@ -200,14 +200,17 @@
>>               try {
>>                   outDoc = UtilXml.readXmlDocument(buf.toString(), false);
>>               } catch (ParserConfigurationException e) {
>> +                dis.close();
>>                   sock.close();
>>                   throw new GeneralException(e);
>>               } catch (SAXException e) {
>> +                dis.close();
>>                   sock.close();
>>                   throw new GeneralException(e);
>>               }
>>
>>               PcChargeApi out = new PcChargeApi(outDoc);
>> +            dis.close();
>>               sock.close();
>>               return out;
>>           } else {
>>
>> Jacques
>>
>>
>>
>> Le 02/09/2016 à 19:16, Taher Alkhateeb a écrit :
>>
>>> Jacques this is still problematic, you are closing the streams upon
>>> exceptions! now all kinds of memory leaks and side effects could take
>>> place
>>> because you are not ensuring closing all streams properly.
>>>
>>> This needs a revert and a full review to make things tight in terms of
>>> memory.
>>>
>>> On Fri, Sep 2, 2016 at 8:08 PM, Jacques Le Roux <
>>> [hidden email]> wrote:
>>>
>>> Le 02/09/2016 à 18:46, Jacopo Cappellato a écrit :
>>>> On Fri, Sep 2, 2016 at 3:19 PM, Jacques Le Roux <
>>>>> [hidden email]> wrote:
>>>>>
>>>>> Le 02/09/2016 à 12:30, Jacopo Cappellato a écrit :
>>>>>
>>>>>> ...
>>>>>> 2) I suspect that you are closing the socket connection too early
>>>>>>
>>>>>> in PcChargeApi
>>>>>>> I see no problem with this point, the "sock" socket connection is not
>>>>>>>
>>>>>> used
>>>>>> in code below.
>>>>>> ...
>>>>>> Jacques
>>>>>>
>>>>>>
>>>>>> Jacques,
>>>>>>
>>>>> at the moment I don't have time to explain but after your response
>>>>> above I
>>>>> am a bit concerned about the effort that you are carrying on, because
>>>>> you
>>>>> may break other code without even suspecting it.
>>>>> I have to go now, if others can explain then great, otherwise I will
>>>>> follow
>>>>> up with you asap.
>>>>>
>>>>> Jacopo
>>>>>
>>>>> Ah I think I got your point when back, I should better close the Socket
>>>>>
>>>> later, done at r1758990
>>>>
>>>> Jacques
>>>>
>>>>
>>>>

[taher]

Hi Jacques,

What you are closing is socket, which is the lowest abstraction in the
chained streams. Java automatically closes from the highest streams chained
through constructors, not lowest, and hence leaving streams in memory, This
is a very common source of memory leaks in Java and a basic thing to care
for when working with closable streams. It is so popular that to reduce
code verbosity in Java 7 they introduced AutoClosable as a parent to
Closable to be used with try-with-resources blocks to ease the work.

Also, the code is now a jumbled code, you're closing twice upon an
exception, this does not have an effect but is a sign of problematic code.
The whole code block can be improved substantially. That is why I am
suggesting a rewrite after a thorough review.



On Sat, Sep 3, 2016 at 12:31 PM, Jacques Le Roux <
[hidden email]> wrote:

> Hi Taher, Jacopo,
>
> Sincerely reading https://docs.oracle.com/javase
> /8/docs/api/java/net/Socket.html#close--
>
> /<<public void close()
>            throws IOException
> Closes this socket.
> Any thread currently blocked in an I/O operation upon this socket will
> throw a SocketException.
> Once a socket has been closed, it is not available for further networking
> use (i.e. can't be reconnected or rebound). A new socket needs to be
> created.
> Closing this socket will also close the socket's InputStream and
> OutputStream.
> If this socket has an associated channel then the channel is closed as
> well.>>
> /
>
> the only problem I can see is if several threads would simultaneously
> access the same PcChargeApi object and its inner Socket has been closed
> then a SocketException will be fired./
> /
>
> But in the PcChargeServices class, each call to PcChargeApi.send() is done
> after the creation of a local PcChargeApi object through
> PcChargeApi.getApi(). So I can't see how this object could be shared.
>
> Hence closing the socket, as I did in my last commit, should not "leads to
> possible memory leaks". By closing it I close also the socket's InputStream
> so no leaks should be expected.
>
> BTW you may wonder (I did!), because PcChargeApi.getApi() returns an
> escaped PcChargeApi object. So it's not thread safe, and another thread
> could access it concurrently (being a PcChargeServices.ccAuth() or any
> other of the PcChargeServices.cc*() public methods)
>
> And if several threads simultaneously access the same PcChargeApi object
> and its Socket has been closed, then a SocketException would be fired.
>
> But, because the Socket does not escape from the PcChargeApi.send(), the
> Socket is thread safe and can't be shared between threads (even if the
> PcChargeApi object can be) and I don't see how a SocketException could be
> fired///
> /
>
> So I see no problem with my currently committed solution.  I initially
> missed to close the Socket in the catched exceptions which is why, I
> believe, Jacopo was concerned.
>
> If I miss something please explain
>
> Note that in the original code the socket was not closed, so a resource
> leak existed.
> It's not a big deal if you don't use a lot of calls to PcChargeApi.send()
> during the JVM session, since the JVM will clear it when finished.
> But it seems to me that it's better to avoid crossing this issue.
>
> And last but not least, this is in a peripheral thirdparty\gosoftware, so
> there is currently nothing to urgently worry about in trunk. We can
> exchange and I'd be happy to stand corrected if I get a clear answer.
>
> Jacques
>
>
>
> Le 02/09/2016 à 20:43, Taher Alkhateeb a écrit :
>
>> Hi Jacques,
>>
>> The resources are not properly closed in your commit. It would take time
>> to
>> explain, suffice to say that your code leads to possible memory leaks.
>> That
>> is why I suggest a full review and carefully applying the stream memory
>> management. There are multiple ways to fix this, some related to Java 7,
>> but it would take time to explain, and it is your initiative so I leave it
>> up to you to fix it.
>>
>> Regards,
>>
>> Taher Alkhateeb
>>
>> On Fri, Sep 2, 2016 at 8:37 PM, Jacques Le Roux <
>> [hidden email]> wrote:
>>
>> Why should I not close the Socket once the DataInputStream derived from it
>>> has been used?
>>>
>>> In the case of an exception occurs will not the resource be leaked?
>>>
>>> Do you mean that I should also close the DataInputStream before closing
>>> the Socket?
>>>
>>> ie something like
>>>
>>> Index: applications/accounting/src/main/java/org/apache/ofbiz/accou
>>> nting/thirdparty/gosoftware/PcChargeApi.java
>>> ===================================================================
>>> --- applications/accounting/src/main/java/org/apache/ofbiz/accou
>>> nting/thirdparty/gosoftware/PcChargeApi.java (revision 1758990)
>>> +++ applications/accounting/src/main/java/org/apache/ofbiz/accou
>>> nting/thirdparty/gosoftware/PcChargeApi.java (working copy)
>>> @@ -200,14 +200,17 @@
>>>               try {
>>>                   outDoc = UtilXml.readXmlDocument(buf.toString(),
>>> false);
>>>               } catch (ParserConfigurationException e) {
>>> +                dis.close();
>>>                   sock.close();
>>>                   throw new GeneralException(e);
>>>               } catch (SAXException e) {
>>> +                dis.close();
>>>                   sock.close();
>>>                   throw new GeneralException(e);
>>>               }
>>>
>>>               PcChargeApi out = new PcChargeApi(outDoc);
>>> +            dis.close();
>>>               sock.close();
>>>               return out;
>>>           } else {
>>>
>>> Jacques
>>>
>>>
>>>
>>> Le 02/09/2016 à 19:16, Taher Alkhateeb a écrit :
>>>
>>> Jacques this is still problematic, you are closing the streams upon
>>>> exceptions! now all kinds of memory leaks and side effects could take
>>>> place
>>>> because you are not ensuring closing all streams properly.
>>>>
>>>> This needs a revert and a full review to make things tight in terms of
>>>> memory.
>>>>
>>>> On Fri, Sep 2, 2016 at 8:08 PM, Jacques Le Roux <
>>>> [hidden email]> wrote:
>>>>
>>>> Le 02/09/2016 à 18:46, Jacopo Cappellato a écrit :
>>>>
>>>>> On Fri, Sep 2, 2016 at 3:19 PM, Jacques Le Roux <
>>>>>
>>>>>> [hidden email]> wrote:
>>>>>>
>>>>>> Le 02/09/2016 à 12:30, Jacopo Cappellato a écrit :
>>>>>>
>>>>>> ...
>>>>>>> 2) I suspect that you are closing the socket connection too early
>>>>>>>
>>>>>>> in PcChargeApi
>>>>>>>
>>>>>>>> I see no problem with this point, the "sock" socket connection is
>>>>>>>> not
>>>>>>>>
>>>>>>>> used
>>>>>>> in code below.
>>>>>>> ...
>>>>>>> Jacques
>>>>>>>
>>>>>>>
>>>>>>> Jacques,
>>>>>>>
>>>>>>> at the moment I don't have time to explain but after your response
>>>>>> above I
>>>>>> am a bit concerned about the effort that you are carrying on, because
>>>>>> you
>>>>>> may break other code without even suspecting it.
>>>>>> I have to go now, if others can explain then great, otherwise I will
>>>>>> follow
>>>>>> up with you asap.
>>>>>>
>>>>>> Jacopo
>>>>>>
>>>>>> Ah I think I got your point when back, I should better close the
>>>>>> Socket
>>>>>>
>>>>>> later, done at r1758990
>>>>>
>>>>> Jacques
>>>>>
>>>>>
>>>>>
>>>>>
>

[Jacques Le Roux]

Thanks Taher,

When I read your answer I thought that then the Oracle JavaDoc below is a least misleading. I mean specifically

<<Closing this socket will also close the socket's InputStream andOutputStream.>>

They should have spoken about possible memory leaks :/

But it seems this area is a bit touchy (just Google for "java does closing socket close streams"!) and I understand your point

Actually I should have used my own advice (to use the try-with-resources statement) from start on this case also.

That's done at revision: 1759082 . I'll review the other changes I did then to check I have not missed a similar case.

Jacques


Le 03/09/2016 à 12:20, Taher Alkhateeb a écrit :

> Hi Jacques,
>
> What you are closing is socket, which is the lowest abstraction in the
> chained streams. Java automatically closes from the highest streams chained
> through constructors, not lowest, and hence leaving streams in memory, This
> is a very common source of memory leaks in Java and a basic thing to care
> for when working with closable streams. It is so popular that to reduce
> code verbosity in Java 7 they introduced AutoClosable as a parent to
> Closable to be used with try-with-resources blocks to ease the work.
>
> Also, the code is now a jumbled code, you're closing twice upon an
> exception, this does not have an effect but is a sign of problematic code.
> The whole code block can be improved substantially. That is why I am
> suggesting a rewrite after a thorough review.
>
>
>
> On Sat, Sep 3, 2016 at 12:31 PM, Jacques Le Roux <
> [hidden email]> wrote:
>
>> Hi Taher, Jacopo,
>>
>> Sincerely reading https://docs.oracle.com/javase
>> /8/docs/api/java/net/Socket.html#close--
>>
>> /<<public void close()
>>             throws IOException
>> Closes this socket.
>> Any thread currently blocked in an I/O operation upon this socket will
>> throw a SocketException.
>> Once a socket has been closed, it is not available for further networking
>> use (i.e. can't be reconnected or rebound). A new socket needs to be
>> created.
>> Closing this socket will also close the socket's InputStream and
>> OutputStream.
>> If this socket has an associated channel then the channel is closed as
>> well.>>
>> /
>>
>> the only problem I can see is if several threads would simultaneously
>> access the same PcChargeApi object and its inner Socket has been closed
>> then a SocketException will be fired./
>> /
>>
>> But in the PcChargeServices class, each call to PcChargeApi.send() is done
>> after the creation of a local PcChargeApi object through
>> PcChargeApi.getApi(). So I can't see how this object could be shared.
>>
>> Hence closing the socket, as I did in my last commit, should not "leads to
>> possible memory leaks". By closing it I close also the socket's InputStream
>> so no leaks should be expected.
>>
>> BTW you may wonder (I did!), because PcChargeApi.getApi() returns an
>> escaped PcChargeApi object. So it's not thread safe, and another thread
>> could access it concurrently (being a PcChargeServices.ccAuth() or any
>> other of the PcChargeServices.cc*() public methods)
>>
>> And if several threads simultaneously access the same PcChargeApi object
>> and its Socket has been closed, then a SocketException would be fired.
>>
>> But, because the Socket does not escape from the PcChargeApi.send(), the
>> Socket is thread safe and can't be shared between threads (even if the
>> PcChargeApi object can be) and I don't see how a SocketException could be
>> fired///
>> /
>>
>> So I see no problem with my currently committed solution.  I initially
>> missed to close the Socket in the catched exceptions which is why, I
>> believe, Jacopo was concerned.
>>
>> If I miss something please explain
>>
>> Note that in the original code the socket was not closed, so a resource
>> leak existed.
>> It's not a big deal if you don't use a lot of calls to PcChargeApi.send()
>> during the JVM session, since the JVM will clear it when finished.
>> But it seems to me that it's better to avoid crossing this issue.
>>
>> And last but not least, this is in a peripheral thirdparty\gosoftware, so
>> there is currently nothing to urgently worry about in trunk. We can
>> exchange and I'd be happy to stand corrected if I get a clear answer.
>>
>> Jacques
>>
>>
>>
>> Le 02/09/2016 à 20:43, Taher Alkhateeb a écrit :
>>
>>> Hi Jacques,
>>>
>>> The resources are not properly closed in your commit. It would take time
>>> to
>>> explain, suffice to say that your code leads to possible memory leaks.
>>> That
>>> is why I suggest a full review and carefully applying the stream memory
>>> management. There are multiple ways to fix this, some related to Java 7,
>>> but it would take time to explain, and it is your initiative so I leave it
>>> up to you to fix it.
>>>
>>> Regards,
>>>
>>> Taher Alkhateeb
>>>
>>> On Fri, Sep 2, 2016 at 8:37 PM, Jacques Le Roux <
>>> [hidden email]> wrote:
>>>
>>> Why should I not close the Socket once the DataInputStream derived from it
>>>> has been used?
>>>>
>>>> In the case of an exception occurs will not the resource be leaked?
>>>>
>>>> Do you mean that I should also close the DataInputStream before closing
>>>> the Socket?
>>>>
>>>> ie something like
>>>>
>>>> Index: applications/accounting/src/main/java/org/apache/ofbiz/accou
>>>> nting/thirdparty/gosoftware/PcChargeApi.java
>>>> ===================================================================
>>>> --- applications/accounting/src/main/java/org/apache/ofbiz/accou
>>>> nting/thirdparty/gosoftware/PcChargeApi.java (revision 1758990)
>>>> +++ applications/accounting/src/main/java/org/apache/ofbiz/accou
>>>> nting/thirdparty/gosoftware/PcChargeApi.java (working copy)
>>>> @@ -200,14 +200,17 @@
>>>>                try {
>>>>                    outDoc = UtilXml.readXmlDocument(buf.toString(),
>>>> false);
>>>>                } catch (ParserConfigurationException e) {
>>>> +                dis.close();
>>>>                    sock.close();
>>>>                    throw new GeneralException(e);
>>>>                } catch (SAXException e) {
>>>> +                dis.close();
>>>>                    sock.close();
>>>>                    throw new GeneralException(e);
>>>>                }
>>>>
>>>>                PcChargeApi out = new PcChargeApi(outDoc);
>>>> +            dis.close();
>>>>                sock.close();
>>>>                return out;
>>>>            } else {
>>>>
>>>> Jacques
>>>>
>>>>
>>>>
>>>> Le 02/09/2016 à 19:16, Taher Alkhateeb a écrit :
>>>>
>>>> Jacques this is still problematic, you are closing the streams upon
>>>>> exceptions! now all kinds of memory leaks and side effects could take
>>>>> place
>>>>> because you are not ensuring closing all streams properly.
>>>>>
>>>>> This needs a revert and a full review to make things tight in terms of
>>>>> memory.
>>>>>
>>>>> On Fri, Sep 2, 2016 at 8:08 PM, Jacques Le Roux <
>>>>> [hidden email]> wrote:
>>>>>
>>>>> Le 02/09/2016 à 18:46, Jacopo Cappellato a écrit :
>>>>>
>>>>>> On Fri, Sep 2, 2016 at 3:19 PM, Jacques Le Roux <
>>>>>>
>>>>>>> [hidden email]> wrote:
>>>>>>>
>>>>>>> Le 02/09/2016 à 12:30, Jacopo Cappellato a écrit :
>>>>>>>
>>>>>>> ...
>>>>>>>> 2) I suspect that you are closing the socket connection too early
>>>>>>>>
>>>>>>>> in PcChargeApi
>>>>>>>>
>>>>>>>>> I see no problem with this point, the "sock" socket connection is
>>>>>>>>> not
>>>>>>>>>
>>>>>>>>> used
>>>>>>>> in code below.
>>>>>>>> ...
>>>>>>>> Jacques
>>>>>>>>
>>>>>>>>
>>>>>>>> Jacques,
>>>>>>>>
>>>>>>>> at the moment I don't have time to explain but after your response
>>>>>>> above I
>>>>>>> am a bit concerned about the effort that you are carrying on, because
>>>>>>> you
>>>>>>> may break other code without even suspecting it.
>>>>>>> I have to go now, if others can explain then great, otherwise I will
>>>>>>> follow
>>>>>>> up with you asap.
>>>>>>>
>>>>>>> Jacopo
>>>>>>>
>>>>>>> Ah I think I got your point when back, I should better close the
>>>>>>> Socket
>>>>>>>
>>>>>>> later, done at r1758990
>>>>>> Jacques
>>>>>>
>>>>>>
>>>>>>
>>>>>>

[taher]

Hi Jacques,

I don't see anything wrong with Oracle's documentation. It is very much
correct that closing a socket closes its streams. The problem is with
closing the higher chained streams.

Taher Alkhateeb

On Sep 3, 2016 3:16 PM, "Jacques Le Roux" <[hidden email]>
wrote:

> Thanks Taher,
>
> When I read your answer I thought that then the Oracle JavaDoc below is a
> least misleading. I mean specifically
>
> <<Closing this socket will also close the socket's InputStream
> andOutputStream.>>
>
> They should have spoken about possible memory leaks :/
>
> But it seems this area is a bit touchy (just Google for "java does closing
> socket close streams"!) and I understand your point
>
> Actually I should have used my own advice (to use the try-with-resources
> statement) from start on this case also.
>
> That's done at revision: 1759082 . I'll review the other changes I did
> then to check I have not missed a similar case.
>
> Jacques
>
>
> Le 03/09/2016 à 12:20, Taher Alkhateeb a écrit :
>
>> Hi Jacques,
>>
>> What you are closing is socket, which is the lowest abstraction in the
>> chained streams. Java automatically closes from the highest streams
>> chained
>> through constructors, not lowest, and hence leaving streams in memory,
>> This
>> is a very common source of memory leaks in Java and a basic thing to care
>> for when working with closable streams. It is so popular that to reduce
>> code verbosity in Java 7 they introduced AutoClosable as a parent to
>> Closable to be used with try-with-resources blocks to ease the work.
>>
>> Also, the code is now a jumbled code, you're closing twice upon an
>> exception, this does not have an effect but is a sign of problematic code.
>> The whole code block can be improved substantially. That is why I am
>> suggesting a rewrite after a thorough review.
>>
>>
>>
>> On Sat, Sep 3, 2016 at 12:31 PM, Jacques Le Roux <
>> [hidden email]> wrote:
>>
>> Hi Taher, Jacopo,
>>>
>>> Sincerely reading https://docs.oracle.com/javase
>>> /8/docs/api/java/net/Socket.html#close--
>>>
>>> /<<public void close()
>>>             throws IOException
>>> Closes this socket.
>>> Any thread currently blocked in an I/O operation upon this socket will
>>> throw a SocketException.
>>> Once a socket has been closed, it is not available for further networking
>>> use (i.e. can't be reconnected or rebound). A new socket needs to be
>>> created.
>>> Closing this socket will also close the socket's InputStream and
>>> OutputStream.
>>> If this socket has an associated channel then the channel is closed as
>>> well.>>
>>> /
>>>
>>> the only problem I can see is if several threads would simultaneously
>>> access the same PcChargeApi object and its inner Socket has been closed
>>> then a SocketException will be fired./
>>> /
>>>
>>> But in the PcChargeServices class, each call to PcChargeApi.send() is
>>> done
>>> after the creation of a local PcChargeApi object through
>>> PcChargeApi.getApi(). So I can't see how this object could be shared.
>>>
>>> Hence closing the socket, as I did in my last commit, should not "leads
>>> to
>>> possible memory leaks". By closing it I close also the socket's
>>> InputStream
>>> so no leaks should be expected.
>>>
>>> BTW you may wonder (I did!), because PcChargeApi.getApi() returns an
>>> escaped PcChargeApi object. So it's not thread safe, and another thread
>>> could access it concurrently (being a PcChargeServices.ccAuth() or any
>>> other of the PcChargeServices.cc*() public methods)
>>>
>>> And if several threads simultaneously access the same PcChargeApi object
>>> and its Socket has been closed, then a SocketException would be fired.
>>>
>>> But, because the Socket does not escape from the PcChargeApi.send(), the
>>> Socket is thread safe and can't be shared between threads (even if the
>>> PcChargeApi object can be) and I don't see how a SocketException could be
>>> fired///
>>> /
>>>
>>> So I see no problem with my currently committed solution.  I initially
>>> missed to close the Socket in the catched exceptions which is why, I
>>> believe, Jacopo was concerned.
>>>
>>> If I miss something please explain
>>>
>>> Note that in the original code the socket was not closed, so a resource
>>> leak existed.
>>> It's not a big deal if you don't use a lot of calls to PcChargeApi.send()
>>> during the JVM session, since the JVM will clear it when finished.
>>> But it seems to me that it's better to avoid crossing this issue.
>>>
>>> And last but not least, this is in a peripheral thirdparty\gosoftware, so
>>> there is currently nothing to urgently worry about in trunk. We can
>>> exchange and I'd be happy to stand corrected if I get a clear answer.
>>>
>>> Jacques
>>>
>>>
>>>
>>> Le 02/09/2016 à 20:43, Taher Alkhateeb a écrit :
>>>
>>> Hi Jacques,
>>>>
>>>> The resources are not properly closed in your commit. It would take time
>>>> to
>>>> explain, suffice to say that your code leads to possible memory leaks.
>>>> That
>>>> is why I suggest a full review and carefully applying the stream memory
>>>> management. There are multiple ways to fix this, some related to Java 7,
>>>> but it would take time to explain, and it is your initiative so I leave
>>>> it
>>>> up to you to fix it.
>>>>
>>>> Regards,
>>>>
>>>> Taher Alkhateeb
>>>>
>>>> On Fri, Sep 2, 2016 at 8:37 PM, Jacques Le Roux <
>>>> [hidden email]> wrote:
>>>>
>>>> Why should I not close the Socket once the DataInputStream derived from
>>>> it
>>>>
>>>>> has been used?
>>>>>
>>>>> In the case of an exception occurs will not the resource be leaked?
>>>>>
>>>>> Do you mean that I should also close the DataInputStream before closing
>>>>> the Socket?
>>>>>
>>>>> ie something like
>>>>>
>>>>> Index: applications/accounting/src/main/java/org/apache/ofbiz/accou
>>>>> nting/thirdparty/gosoftware/PcChargeApi.java
>>>>> ===================================================================
>>>>> --- applications/accounting/src/main/java/org/apache/ofbiz/accou
>>>>> nting/thirdparty/gosoftware/PcChargeApi.java (revision 1758990)
>>>>> +++ applications/accounting/src/main/java/org/apache/ofbiz/accou
>>>>> nting/thirdparty/gosoftware/PcChargeApi.java (working copy)
>>>>> @@ -200,14 +200,17 @@
>>>>>                try {
>>>>>                    outDoc = UtilXml.readXmlDocument(buf.toString(),
>>>>> false);
>>>>>                } catch (ParserConfigurationException e) {
>>>>> +                dis.close();
>>>>>                    sock.close();
>>>>>                    throw new GeneralException(e);
>>>>>                } catch (SAXException e) {
>>>>> +                dis.close();
>>>>>                    sock.close();
>>>>>                    throw new GeneralException(e);
>>>>>                }
>>>>>
>>>>>                PcChargeApi out = new PcChargeApi(outDoc);
>>>>> +            dis.close();
>>>>>                sock.close();
>>>>>                return out;
>>>>>            } else {
>>>>>
>>>>> Jacques
>>>>>
>>>>>
>>>>>
>>>>> Le 02/09/2016 à 19:16, Taher Alkhateeb a écrit :
>>>>>
>>>>> Jacques this is still problematic, you are closing the streams upon
>>>>>
>>>>>> exceptions! now all kinds of memory leaks and side effects could take
>>>>>> place
>>>>>> because you are not ensuring closing all streams properly.
>>>>>>
>>>>>> This needs a revert and a full review to make things tight in terms of
>>>>>> memory.
>>>>>>
>>>>>> On Fri, Sep 2, 2016 at 8:08 PM, Jacques Le Roux <
>>>>>> [hidden email]> wrote:
>>>>>>
>>>>>> Le 02/09/2016 à 18:46, Jacopo Cappellato a écrit :
>>>>>>
>>>>>> On Fri, Sep 2, 2016 at 3:19 PM, Jacques Le Roux <
>>>>>>>
>>>>>>> [hidden email]> wrote:
>>>>>>>>
>>>>>>>> Le 02/09/2016 à 12:30, Jacopo Cappellato a écrit :
>>>>>>>>
>>>>>>>> ...
>>>>>>>>
>>>>>>>>> 2) I suspect that you are closing the socket connection too early
>>>>>>>>>
>>>>>>>>> in PcChargeApi
>>>>>>>>>
>>>>>>>>> I see no problem with this point, the "sock" socket connection is
>>>>>>>>>> not
>>>>>>>>>>
>>>>>>>>>> used
>>>>>>>>>>
>>>>>>>>> in code below.
>>>>>>>>> ...
>>>>>>>>> Jacques
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Jacques,
>>>>>>>>>
>>>>>>>>> at the moment I don't have time to explain but after your response
>>>>>>>>>
>>>>>>>> above I
>>>>>>>> am a bit concerned about the effort that you are carrying on,
>>>>>>>> because
>>>>>>>> you
>>>>>>>> may break other code without even suspecting it.
>>>>>>>> I have to go now, if others can explain then great, otherwise I will
>>>>>>>> follow
>>>>>>>> up with you asap.
>>>>>>>>
>>>>>>>> Jacopo
>>>>>>>>
>>>>>>>> Ah I think I got your point when back, I should better close the
>>>>>>>> Socket
>>>>>>>>
>>>>>>>> later, done at r1758990
>>>>>>>>
>>>>>>> Jacques
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>

[Jacques Le Roux]

Le 03/09/2016 à 14:16, Jacques Le Roux a écrit :
> I'll review the other changes I did then to check I have not missed a similar case.

Done at r1759088, no functional changes, I simply used the try-with-resources statement everywhere it was possible in the classes I touched before,
mostly to clean the finally statements and have consistent code.
I guess there are (a lot?) of other places which would support the same treatment, but it's not worth it for now.

Jacques