James Server
  1. James Server
  2. JAMES-474

NullPointerException and bodymessage lost with weird configurations in message spooling

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.3.0
    • Fix Version/s: 2.3.0
    • Component/s: James Core
    • Labels:
      None

      Description

      Under particular condition the new MimeMessageWrapper optimization code seems to loose the message source.
      I've already created a unittest for this and I remove the optimization, but this need a fix.

        Issue Links

          Activity

          Stefano Bagnara created issue -
          Hide
          Martijn Brinkers added a comment -

          Should'nt access to some (or all?) variables like modified, bodyModified etc. be synchronized?

          The method:

          public synchronized void setDataHandler(DataHandler arg0) throws MessagingException

          changes modified and bodyModified and it's synchonized but all other access to modified and bodyModified is not synchronized. The constructor for example checks bodyModified but access to bodyModified is not synchronized. A context switch can occur in setDataHandler just before bodyModified is set to true which results in an 'out-of-sync' between what the constructor 'sees' and what setDataHandler 'sees'.

          A related question. Why are all variables protected and not private using getters/setters? Having them private with getters/setters would make it easier to add thread safety.

          Show
          Martijn Brinkers added a comment - Should'nt access to some (or all?) variables like modified, bodyModified etc. be synchronized? The method: public synchronized void setDataHandler(DataHandler arg0) throws MessagingException changes modified and bodyModified and it's synchonized but all other access to modified and bodyModified is not synchronized. The constructor for example checks bodyModified but access to bodyModified is not synchronized. A context switch can occur in setDataHandler just before bodyModified is set to true which results in an 'out-of-sync' between what the constructor 'sees' and what setDataHandler 'sees'. A related question. Why are all variables protected and not private using getters/setters? Having them private with getters/setters would make it easier to add thread safety.
          Hide
          Stefano Bagnara added a comment -

          Martijn, thank you for your comment.

          This issue is not related to synchronization, btw I think I should review the synchronization in the MimeMessageWrapper because I changed it a lot and I didn't check the synchronizations lately. The problem here is that 2 MimeMessageWrapper ends up sharing a single MimeMessageSource and the MimeMessageSource.dispose() call by one of them remove the source to the other message: I'm deciding wether to remove the optimization (as I did in the "fast fix") or introduce counters to know how much messages share the source.

          I'm not sure that the issue you point about non synchronized property access in the constructor is valid, because I don't see how a content switch can happen during the constructor (can another thread get access to the object before the constructor is completely executed?).

          They are not public, I think that field exists for a reason and getter/setter are not always the best thing. Btw, in this case I simply followed the style used in MimeMessage, as we extend that object and MimeMessage provide access to its internal via protected fields.

          Show
          Stefano Bagnara added a comment - Martijn, thank you for your comment. This issue is not related to synchronization, btw I think I should review the synchronization in the MimeMessageWrapper because I changed it a lot and I didn't check the synchronizations lately. The problem here is that 2 MimeMessageWrapper ends up sharing a single MimeMessageSource and the MimeMessageSource.dispose() call by one of them remove the source to the other message: I'm deciding wether to remove the optimization (as I did in the "fast fix") or introduce counters to know how much messages share the source. I'm not sure that the issue you point about non synchronized property access in the constructor is valid, because I don't see how a content switch can happen during the constructor (can another thread get access to the object before the constructor is completely executed?). They are not public, I think that field exists for a reason and getter/setter are not always the best thing. Btw, in this case I simply followed the style used in MimeMessage, as we extend that object and MimeMessage provide access to its internal via protected fields.
          Hide
          Norman Maurer added a comment -

          Is this error caused by this? And any idea why it is not happen everytime when run the junit test:

          testEmptyMessage(org.apache.james.smtpserver.SMTPServerTest)
          java.lang.NullPointerException
          at org.apache.james.core.MimeMessageCopyOnWriteProxy.getSize(MimeMessageCopyOnWriteProxy.java:234)
          at org.apache.james.smtpserver.SMTPServerTest.testEmptyMessage(SMTPServerTest.java:197)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at java.lang.reflect.Method.invoke(Method.java:585)
          at junit.framework.TestCase.runTest(TestCase.java:164)
          at junit.framework.TestCase.runBare(TestCase.java:130)
          at junit.framework.TestResult$1.protect(TestResult.java:110)
          at junit.framework.TestResult.runProtected(TestResult.java:128)
          at junit.framework.TestResult.run(TestResult.java:113)
          at junit.framework.TestCase.run(TestCase.java:120)
          at junit.framework.TestSuite.runTest(TestSuite.java:228)
          at junit.framework.TestSuite.run(TestSuite.java:223)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:478)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:344)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:196)

          Show
          Norman Maurer added a comment - Is this error caused by this? And any idea why it is not happen everytime when run the junit test: testEmptyMessage(org.apache.james.smtpserver.SMTPServerTest) java.lang.NullPointerException at org.apache.james.core.MimeMessageCopyOnWriteProxy.getSize(MimeMessageCopyOnWriteProxy.java:234) at org.apache.james.smtpserver.SMTPServerTest.testEmptyMessage(SMTPServerTest.java:197) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:585) at junit.framework.TestCase.runTest(TestCase.java:164) at junit.framework.TestCase.runBare(TestCase.java:130) at junit.framework.TestResult$1.protect(TestResult.java:110) at junit.framework.TestResult.runProtected(TestResult.java:128) at junit.framework.TestResult.run(TestResult.java:113) at junit.framework.TestCase.run(TestCase.java:120) at junit.framework.TestSuite.runTest(TestSuite.java:228) at junit.framework.TestSuite.run(TestSuite.java:223) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:478) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:344) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:196)
          Hide
          Stefano Bagnara added a comment -

          Adding a "System.gc();" before the getSize I'm able to reproduce your NPE.
          I will check the problem as soon as possible.

          Show
          Stefano Bagnara added a comment - Adding a "System.gc();" before the getSize I'm able to reproduce your NPE. I will check the problem as soon as possible.
          Hide
          Stefano Bagnara added a comment -

          The problem is in the SMTPServerTest: SMTPHandler dispose the Mail as soon as it processed it. The current test implementation doesn't clone the message when receiving it but share the message with the SMTPHandler. When SMTPHandler dispose the MailImpl object, in turn the MimeMessageCopyOnWriteProxy.dispose is called and the wrapped mimemessage is set to null.

          Show
          Stefano Bagnara added a comment - The problem is in the SMTPServerTest: SMTPHandler dispose the Mail as soon as it processed it. The current test implementation doesn't clone the message when receiving it but share the message with the SMTPHandler. When SMTPHandler dispose the MailImpl object, in turn the MimeMessageCopyOnWriteProxy.dispose is called and the wrapped mimemessage is set to null.
          Stefano Bagnara made changes -
          Field Original Value New Value
          Priority Critical [ 2 ] Blocker [ 1 ]
          Stefano Bagnara made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Stefano Bagnara made changes -
          Fix Version/s 2.3.0a2 [ 12310795 ]
          Hide
          Stefano Bagnara added a comment -

          Marking this as resolved because the current trunk should no more been affected by this bug.
          Btw we should link to this issue and open a new "Improvement" issue to find a solution to reenable the optimizations provided by the "commented" code.

          Show
          Stefano Bagnara added a comment - Marking this as resolved because the current trunk should no more been affected by this bug. Btw we should link to this issue and open a new "Improvement" issue to find a solution to reenable the optimizations provided by the "commented" code.
          Stefano Bagnara made changes -
          Status In Progress [ 3 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Stefano Bagnara added a comment -

          Not sure this bug was opened for this exact NPE, but I found a new problem while working on the Handler refactoring, added tests and a possible fix.

          I reopen this because this probably should be merged on 2.3.0a2 before the release, but need more testing.

          Show
          Stefano Bagnara added a comment - Not sure this bug was opened for this exact NPE, but I found a new problem while working on the Handler refactoring, added tests and a possible fix. I reopen this because this probably should be merged on 2.3.0a2 before the release, but need more testing.
          Stefano Bagnara made changes -
          Status Resolved [ 5 ] Reopened [ 4 ]
          Resolution Fixed [ 1 ]
          Hide
          Stefano Bagnara added a comment -

          Let's say this is closed now.

          Show
          Stefano Bagnara added a comment - Let's say this is closed now.
          Stefano Bagnara made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Fix Version/s 2.3.0a3 [ 12311010 ]
          Fix Version/s 2.3.0a2 [ 12310795 ]
          Resolution Fixed [ 1 ]
          Hide
          Norman Maurer added a comment -

          After upgrading from james-2.3a2 to james-2.3.0b1 we see again that the headers get lost after do changes on headers. So the bug is probaly still here
          We wrote a mailet to log all headers and it shows that the headers get truncated after the fist change of headers is done in a mailet!

          We tested it with mysql-4.1 and mysql conector 3.1.13 and 5.0-beta. Os is debian ubuntu with java 1.5

          Show
          Norman Maurer added a comment - After upgrading from james-2.3a2 to james-2.3.0b1 we see again that the headers get lost after do changes on headers. So the bug is probaly still here We wrote a mailet to log all headers and it shows that the headers get truncated after the fist change of headers is done in a mailet! We tested it with mysql-4.1 and mysql conector 3.1.13 and 5.0-beta. Os is debian ubuntu with java 1.5
          Norman Maurer made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Hide
          Stefano Bagnara added a comment -

          The problem was not related to this issue.
          I thought this at first, too.. but I think it was there previously. Opened JAMES-538 for the "new" problem

          Show
          Stefano Bagnara added a comment - The problem was not related to this issue. I thought this at first, too.. but I think it was there previously. Opened JAMES-538 for the "new" problem
          Stefano Bagnara made changes -
          Resolution Fixed [ 1 ]
          Status Reopened [ 4 ] Resolved [ 5 ]
          Hide
          Danny Angus added a comment -

          Closing issue fixed in released version.

          Show
          Danny Angus added a comment - Closing issue fixed in released version.
          Danny Angus made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Norman Maurer made changes -
          Link This issue relates to JAMES-1095 [ JAMES-1095 ]
          Mark Thomas made changes -
          Workflow jira [ 12353546 ] Default workflow, editable Closed status [ 12566570 ]
          Mark Thomas made changes -
          Workflow Default workflow, editable Closed status [ 12566570 ] jira [ 12582117 ]

            People

            • Assignee:
              Stefano Bagnara
              Reporter:
              Stefano Bagnara
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development