You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by bt...@apache.org on 2021/02/23 03:45:25 UTC
[james-project] 12/14: JAMES-3477 Avoid unneeded copies of
MimeMessages
This is an automated email from the ASF dual-hosted git repository.
btellier pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 38396032fe8aa6002dd2420bcf2b69ddf0f5e816
Author: Benoit Tellier <bt...@linagora.com>
AuthorDate: Sat Jan 2 11:05:02 2021 +0700
JAMES-3477 Avoid unneeded copies of MimeMessages
We needlessly copy mime messages in a few occasions:
- After receiving an email via SMTP and enqueuing it
- When instantiating a Mail via JMAP
- When dequeuing a mail
In all these cases there is no need to proceed with the defensive copy
in MailImpl::setMail as we just instanciated it and won't keep a reference.
The COW approach did hide this pitfall.
---
server/blob/mail-store/pom.xml | 4 ++
.../apache/james/blob/mail/MimeMessageStore.java | 47 +++++++++++++++++-----
.../org/apache/james/server/core/MailImpl.java | 10 ++++-
.../james/jmap/draft/methods/MessageSender.java | 16 ++------
.../jmap/method/EmailSubmissionSetMethod.scala | 20 +++++----
.../DataLineJamesMessageHookHandler.java | 6 +--
.../apache/james/queue/rabbitmq/MailLoader.java | 11 ++++-
7 files changed, 75 insertions(+), 39 deletions(-)
diff --git a/server/blob/mail-store/pom.xml b/server/blob/mail-store/pom.xml
index 32f8268..dbb7261 100644
--- a/server/blob/mail-store/pom.xml
+++ b/server/blob/mail-store/pom.xml
@@ -52,6 +52,10 @@
</dependency>
<dependency>
<groupId>${james.groupId}</groupId>
+ <artifactId>james-server-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>${james.groupId}</groupId>
<artifactId>james-server-util</artifactId>
</dependency>
<dependency>
diff --git a/server/blob/mail-store/src/main/java/org/apache/james/blob/mail/MimeMessageStore.java b/server/blob/mail-store/src/main/java/org/apache/james/blob/mail/MimeMessageStore.java
index 7614928..f2eb24b 100644
--- a/server/blob/mail-store/src/main/java/org/apache/james/blob/mail/MimeMessageStore.java
+++ b/server/blob/mail-store/src/main/java/org/apache/james/blob/mail/MimeMessageStore.java
@@ -32,12 +32,11 @@ import java.io.InputStream;
import java.io.SequenceInputStream;
import java.nio.ByteBuffer;
import java.util.Map;
-import java.util.Properties;
+import java.util.UUID;
import java.util.stream.Stream;
import javax.inject.Inject;
import javax.mail.MessagingException;
-import javax.mail.Session;
import javax.mail.internet.MimeMessage;
import org.apache.commons.io.IOUtils;
@@ -45,6 +44,8 @@ import org.apache.commons.lang3.tuple.Pair;
import org.apache.james.blob.api.BlobStore;
import org.apache.james.blob.api.BlobType;
import org.apache.james.blob.api.Store;
+import org.apache.james.server.core.MimeMessageSource;
+import org.apache.james.server.core.MimeMessageWrapper;
import org.apache.james.util.io.BodyOffsetInputStream;
import com.google.common.base.Preconditions;
@@ -137,21 +138,45 @@ public class MimeMessageStore {
Preconditions.checkArgument(pairs.containsKey(BODY_BLOB_TYPE));
return toMimeMessage(
- new SequenceInputStream(
- new ByteArrayInputStream(pairs.get(HEADER_BLOB_TYPE)),
- new ByteArrayInputStream(pairs.get(BODY_BLOB_TYPE))));
+ pairs.get(HEADER_BLOB_TYPE),
+ pairs.get(BODY_BLOB_TYPE));
}
- private MimeMessage toMimeMessage(InputStream inputStream) {
- try {
- return new MimeMessage(Session.getInstance(new Properties()), inputStream);
- } catch (MessagingException e) {
- throw new RuntimeException(e);
- }
+ private MimeMessage toMimeMessage(byte[] headers, byte[] body) {
+ return new MimeMessageWrapper(new MimeMessageBytesSource(headers, body));
}
}
public static Factory factory(BlobStore blobStore) {
return new Factory(blobStore);
}
+
+ private static class MimeMessageBytesSource extends MimeMessageSource {
+ private final byte[] headers;
+ private final byte[] body;
+ private final String sourceId;
+
+ private MimeMessageBytesSource(byte[] headers, byte[] body) {
+ this.headers = headers;
+ this.body = body;
+ this.sourceId = UUID.randomUUID().toString();
+ }
+
+ @Override
+ public String getSourceId() {
+ return sourceId;
+ }
+
+ @Override
+ public InputStream getInputStream() {
+ return new SequenceInputStream(
+ new ByteArrayInputStream(headers),
+ new ByteArrayInputStream(body));
+ }
+
+ @Override
+ public long getMessageSize() {
+ return headers.length + body.length;
+ }
+ }
}
diff --git a/server/container/core/src/main/java/org/apache/james/server/core/MailImpl.java b/server/container/core/src/main/java/org/apache/james/server/core/MailImpl.java
index 4a231af..8903a58 100644
--- a/server/container/core/src/main/java/org/apache/james/server/core/MailImpl.java
+++ b/server/container/core/src/main/java/org/apache/james/server/core/MailImpl.java
@@ -495,6 +495,10 @@ public class MailImpl implements Disposable, Mail {
*/
@Override
public void setMessage(MimeMessage message) throws MessagingException {
+ setMessageNoCopy(new MimeMessageWrapper(message));
+ }
+
+ public void setMessageNoCopy(MimeMessageWrapper message) throws MessagingException {
if (this.message != message) {
// If a setMessage is called on a Mail that already have a message
// (discouraged) we have to make sure that the message we remove is
@@ -502,10 +506,14 @@ public class MailImpl implements Disposable, Mail {
if (this.message != null) {
LifecycleUtil.dispose(this.message);
}
- this.message = new MimeMessageWrapper(message);
+ this.message = message;
}
}
+ public void setMessageContent(MimeMessageSource message) throws MessagingException {
+ setMessageNoCopy(new MimeMessageWrapper(message));
+ }
+
@Override
public void setRecipients(Collection<MailAddress> recipients) {
this.recipients = ImmutableList.copyOf(recipients);
diff --git a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/MessageSender.java b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/MessageSender.java
index 5ca5d32..dae8b34 100644
--- a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/MessageSender.java
+++ b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/MessageSender.java
@@ -19,11 +19,8 @@
package org.apache.james.jmap.draft.methods;
-import java.io.InputStream;
-
import javax.inject.Inject;
import javax.mail.MessagingException;
-import javax.mail.internet.MimeMessage;
import org.apache.james.jmap.draft.model.message.view.MessageFullViewFactory.MetaDataWithContent;
import org.apache.james.jmap.draft.send.MailMetadata;
@@ -34,8 +31,6 @@ import org.apache.james.mailbox.model.MessageId;
import org.apache.james.server.core.Envelope;
import org.apache.james.server.core.MailImpl;
import org.apache.james.server.core.MimeMessageInputStreamSource;
-import org.apache.james.server.core.MimeMessageSource;
-import org.apache.james.server.core.MimeMessageWrapper;
import org.apache.mailet.Mail;
import com.google.common.annotations.VisibleForTesting;
@@ -62,18 +57,13 @@ public class MessageSender {
@VisibleForTesting
static Mail buildMail(MetaDataWithContent message, Envelope envelope) throws MessagingException {
String name = message.getMessageId().serialize();
- return MailImpl.builder()
+ MailImpl mail = MailImpl.builder()
.name(name)
.sender(envelope.getFrom().asOptional().orElseThrow(() -> new RuntimeException("Sender is mandatory")))
.addRecipients(envelope.getRecipients())
- .mimeMessage(toMimeMessage(name, message.getContent()))
.build();
- }
-
- private static MimeMessage toMimeMessage(String name, InputStream inputStream) throws MessagingException {
- MimeMessageSource source = new MimeMessageInputStreamSource(name, inputStream);
-
- return new MimeMessageWrapper(source);
+ mail.setMessageContent(new MimeMessageInputStreamSource(name, message.getContent()));
+ return mail;
}
public void sendMessage(MessageId messageId,
diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/EmailSubmissionSetMethod.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/EmailSubmissionSetMethod.scala
index ac097e5..9529f14 100644
--- a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/EmailSubmissionSetMethod.scala
+++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/EmailSubmissionSetMethod.scala
@@ -214,13 +214,17 @@ class EmailSubmissionSetMethod @Inject()(serializer: EmailSubmissionSetSerialize
message <- toMimeMessage(submissionId.value, m.getFullContent.getInputStream)
envelope <- resolveEnvelope(message, request.envelope)
validation <- validate(mailboxSession)(message, envelope)
- enqueue <- Try(queue.enQueue(MailImpl.builder()
- .name(submissionId.value)
- .addRecipients(envelope.rcptTo.map(_.email).asJava)
- .sender(envelope.mailFrom.email)
- .mimeMessage(message)
- .addAttribute(new Attribute(MAIL_METADATA_USERNAME_ATTRIBUTE, AttributeValue.of(mailboxSession.getUser.asString())))
- .build()))
+ mail <- Try({
+ val mailImpl = MailImpl.builder()
+ .name(submissionId.value)
+ .addRecipients(envelope.rcptTo.map(_.email).asJava)
+ .sender(envelope.mailFrom.email)
+ .addAttribute(new Attribute(MAIL_METADATA_USERNAME_ATTRIBUTE, AttributeValue.of(mailboxSession.getUser.asString())))
+ .build()
+ mailImpl.setMessageNoCopy(message)
+ mailImpl
+ })
+ enqueue <- Try(queue.enQueue(mail))
} yield {
EmailSubmissionCreationResponse(submissionId)
}
@@ -228,7 +232,7 @@ class EmailSubmissionSetMethod @Inject()(serializer: EmailSubmissionSetSerialize
})
}
- private def toMimeMessage(name: String, inputStream: InputStream): Try[MimeMessage] = {
+ private def toMimeMessage(name: String, inputStream: InputStream): Try[MimeMessageWrapper] = {
val source = new MimeMessageInputStreamSource(name, inputStream)
// if MimeMessageCopyOnWriteProxy throws an error in the constructor we
// have to manually care disposing our source.
diff --git a/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java b/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java
index e9afa81..0de3915 100644
--- a/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java
+++ b/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java
@@ -51,7 +51,6 @@ import org.apache.james.protocols.smtp.hook.MessageHook;
import org.apache.james.server.core.MailImpl;
import org.apache.james.server.core.MimeMessageInputStream;
import org.apache.james.server.core.MimeMessageInputStreamSource;
-import org.apache.james.server.core.MimeMessageWrapper;
import org.apache.mailet.Mail;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -100,10 +99,8 @@ public class DataLineJamesMessageHookHandler implements DataLineFilter, Extensib
// store mail in the session so we can be sure it get disposed later
session.setAttachment(SMTPConstants.MAIL, mail, State.Transaction);
- MimeMessageWrapper mimeMessageWrapper = null;
try {
- mimeMessageWrapper = new MimeMessageWrapper(mmiss);
- mail.setMessage(mimeMessageWrapper);
+ mail.setMessageContent(mmiss);
Response response = processExtensions(session, mail);
@@ -115,7 +112,6 @@ public class DataLineJamesMessageHookHandler implements DataLineFilter, Extensib
LOGGER.info("Unexpected error handling DATA stream", e);
return new SMTPResponse(SMTPRetCode.LOCAL_ERROR, "Unexpected error handling DATA stream.");
} finally {
- LifecycleUtil.dispose(mimeMessageWrapper);
LifecycleUtil.dispose(mmiss);
LifecycleUtil.dispose(mail);
}
diff --git a/server/queue/queue-rabbitmq/src/main/java/org/apache/james/queue/rabbitmq/MailLoader.java b/server/queue/queue-rabbitmq/src/main/java/org/apache/james/queue/rabbitmq/MailLoader.java
index ffe765b..1548081 100644
--- a/server/queue/queue-rabbitmq/src/main/java/org/apache/james/queue/rabbitmq/MailLoader.java
+++ b/server/queue/queue-rabbitmq/src/main/java/org/apache/james/queue/rabbitmq/MailLoader.java
@@ -29,6 +29,8 @@ import org.apache.james.blob.api.BlobId;
import org.apache.james.blob.api.Store;
import org.apache.james.blob.mail.MimeMessagePartsId;
import org.apache.james.queue.api.MailQueue;
+import org.apache.james.server.core.MailImpl;
+import org.apache.james.server.core.MimeMessageWrapper;
import org.apache.mailet.Mail;
import com.github.fge.lambdas.Throwing;
@@ -60,7 +62,14 @@ class MailLoader {
private Mono<Mail> buildMailWithMessageReference(MailReference mailReference, MimeMessage mimeMessage) {
Function<Mail, Mono<Object>> setMessage = mail ->
- Mono.fromRunnable(Throwing.runnable(() -> mail.setMessage(mimeMessage)).sneakyThrow())
+ Mono.fromRunnable(Throwing.runnable(() -> {
+ if (mimeMessage instanceof MimeMessageWrapper && mail instanceof MailImpl) {
+ MailImpl mailImpl = (MailImpl) mail;
+ mailImpl.setMessageNoCopy((MimeMessageWrapper) mimeMessage);
+ } else {
+ mail.setMessage(mimeMessage);
+ }
+ }).sneakyThrow())
.onErrorResume(AddressException.class, e -> Mono.error(new MailQueue.MailQueueException("Failed to parse mail address", e)))
.onErrorResume(MessagingException.class, e -> Mono.error(new MailQueue.MailQueueException("Failed to generate mime message", e)));
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org