NullPointerException and bodymessage lost with weird configurations in message spooling

• Key: JAMES-474
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Blocker
• Assignee: Stefano Bagnara
• Reporter: Stefano Bagnara
• Component/s: James Core
• Affects Version/s: 2.3.0
• Fix Version/s: 2.3.0

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.

Comments

[Martijn Brinkers]

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.

[Stefano Bagnara]

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.

[Norman Maurer]

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)

[Stefano Bagnara]

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

[Stefano Bagnara]

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]

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]

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]

Let's say this is closed now.

[Norman Maurer]

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

[Stefano Bagnara]

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

[Danny Angus]

Closing issue fixed in released version.