Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-8115

Fixe a bunch of small leaks (closes missing, reported as warnings in Eclipse)

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: 16.11.01
    • Component/s: ALL COMPONENTS
    • Labels:
      None

      Description

      At r1758927 I tried to fixe a bunch of closes missing, reported as warnings in Eclipse

      Jacopo noticed and reported on dev ML http://markmail.org/message/b4hvzi7foftfq3j5 2 possible issues:

      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

      So I reverted r1758927 to check the issues

        Activity

        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        This should be OK now, please reopen if you find an issue.

        Show
        jacques.le.roux Jacques Le Roux added a comment - This should be OK now, please reopen if you find an issue.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        All is fixed after r1759082 and r1759088

        Show
        jacques.le.roux Jacques Le Roux added a comment - All is fixed after r1759082 and r1759088
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Reopening, there maybe still an issue with the point 2

        Show
        jacques.le.roux Jacques Le Roux added a comment - Reopening, there maybe still an issue with the point 2
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        I completed the point 2 at r1758990, better to close the Socket at the last occasions.

        Show
        jacques.le.roux Jacques Le Roux added a comment - I completed the point 2 at r1758990, better to close the Socket at the last occasions.
        Hide
        jacques.le.roux Jacques Le Roux added a comment - - edited

        Point 1)
        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 (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 for that today.

        Done at revision: 1758951

        Show
        jacques.le.roux Jacques Le Roux added a comment - - edited Point 1) 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 (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 for that today. Done at revision: 1758951
        Hide
        jacques.le.roux Jacques Le Roux added a comment - - edited

        I see no problem with the point 2, but the point 1 is more complex (I wonder why Eclipse would be wrong) and I'll double check all details...

        Show
        jacques.le.roux Jacques Le Roux added a comment - - edited I see no problem with the point 2, but the point 1 is more complex (I wonder why Eclipse would be wrong) and I'll double check all details...

          People

          • Assignee:
            jacques.le.roux Jacques Le Roux
            Reporter:
            jacques.le.roux Jacques Le Roux
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development