Fix mail lock race condition & Sonar smells (#2721)

* Fix mail lock race condition & Sonar smells

https://build.spring.io/browse/INT-MASTERSPRING40-599/

* The `folderReadLock` might not be re-locked when `openFolder()`
throws an exception
* Fix all the Sonar smells for mail module
* Optimize all the dynamic `Assert` messages to `Supplier`
* Refactor `MailReceiverFactoryBean` to be based on the `AbstractFactoryBean`

* * Fix new reported Sonar smells

* * Log error during cancel idle state in the `ImapMailReceiver`
* Remove commented code in the `MailTransportUtils`

Author: artembilan

spring-integration-mail/src/main/java/org/springframework/integration/mail/AbstractMailReceiver.java

@@ -25,7 +25,6 @@
 import java.util.Map;
 import java.util.Properties;
 import java.util.concurrent.locks.Lock;
-import java.util.concurrent.locks.ReadWriteLock;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 import javax.mail.Authenticator;
@@ -72,7 +71,7 @@ public abstract class AbstractMailReceiver extends IntegrationObjectSupport impl
 
 	private final URLName url;
 
-	private final ReadWriteLock folderLock = new ReentrantReadWriteLock();
+	private final ReentrantReadWriteLock folderLock = new ReentrantReadWriteLock();
 
 	private final Lock folderReadLock = this.folderLock.readLock();
 
@@ -340,7 +339,7 @@ private Folder obtainFolderInstance() throws MessagingException {
 
 	@Override
 	public Object[] receive() throws javax.mail.MessagingException {
-		this.folderReadLock.lock();
+		this.folderReadLock.lock(); // NOSONAR - guarded with the getReadHoldCount()
 		try {
 			try {
 				Folder folderToCheck = getFolder();
@@ -362,7 +361,9 @@ public Object[] receive() throws javax.mail.MessagingException {
 			}
 		}
 		finally {
-			this.folderReadLock.unlock();
+			if (this.folderLock.getReadHoldCount() > 0) {
+				this.folderReadLock.unlock();
+			}
 		}
 	}
 
@@ -416,22 +417,17 @@ private Object[] convertMessagesIfNecessary(MimeMessage[] filteredMessages) {
 	private Object extractContent(MimeMessage message, Map<String, Object> headers) {
 		Object content;
 		try {
-			MimeMessage theMessage;
+			MimeMessage theMessage = message;
 			if (this.simpleContent) {
 				theMessage = new IntegrationMimeMessage(message);
 			}
-			else {
-				theMessage = message;
-			}
 			content = theMessage.getContent();
 			if (content instanceof String) {
+				headers.put(MessageHeaders.CONTENT_TYPE, "text/plain");
 				String mailContentType = (String) headers.get(MailHeaders.CONTENT_TYPE);
 				if (mailContentType != null && mailContentType.toLowerCase().startsWith("text")) {
 					headers.put(MessageHeaders.CONTENT_TYPE, mailContentType);
 				}
-				else {
-					headers.put(MessageHeaders.CONTENT_TYPE, "text/plain");
-				}
 			}
 			else if (content instanceof InputStream) {
 				ByteArrayOutputStream baos = new ByteArrayOutputStream();

spring-integration-mail/src/main/java/org/springframework/integration/mail/ImapIdleChannelAdapter.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2018 the original author or authors.
+ * Copyright 2002-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -154,7 +154,7 @@ public void setApplicationEventPublisher(ApplicationEventPublisher applicationEv
 
 	@Override // guarded by super#lifecycleLock
 	protected void doStart() {
-		final TaskScheduler scheduler = this.getTaskScheduler();
+		TaskScheduler scheduler = getTaskScheduler();
 		Assert.notNull(scheduler, "'taskScheduler' must not be null");
 		if (this.sendingTaskExecutor == null) {
 			this.sendingTaskExecutor = Executors.newFixedThreadPool(1);
@@ -182,39 +182,8 @@ protected void doStop() {
 		}
 	}
 
-	private Runnable createMessageSendingTask(final Object mailMessage) {
-		Runnable sendingTask = () -> {
-			@SuppressWarnings("unchecked")
-			org.springframework.messaging.Message<?> message =
-					mailMessage instanceof Message
-							? getMessageBuilderFactory().withPayload(mailMessage).build()
-							: (org.springframework.messaging.Message<Object>) mailMessage;
-
-			if (TransactionSynchronizationManager.isActualTransactionActive()) {
-				if (ImapIdleChannelAdapter.this.transactionSynchronizationFactory != null) {
-					TransactionSynchronization synchronization =
-							ImapIdleChannelAdapter.this.transactionSynchronizationFactory
-									.create(ImapIdleChannelAdapter.this);
-					if (synchronization != null) {
-						TransactionSynchronizationManager.registerSynchronization(synchronization);
-
-						if (synchronization instanceof IntegrationResourceHolderSynchronization
-								&& !TransactionSynchronizationManager.hasResource(ImapIdleChannelAdapter.this)) {
-
-							TransactionSynchronizationManager.bindResource(ImapIdleChannelAdapter.this,
-									((IntegrationResourceHolderSynchronization) synchronization).getResourceHolder());
-						}
-
-						Object resourceHolder =
-								TransactionSynchronizationManager.getResource(ImapIdleChannelAdapter.this);
-						if (resourceHolder instanceof IntegrationResourceHolder) {
-							((IntegrationResourceHolder) resourceHolder).setMessage(message);
-						}
-					}
-				}
-			}
-			sendMessage(message);
-		};
+	private Runnable createMessageSendingTask(Object mailMessage) {
+		Runnable sendingTask = prepareSendingTask(mailMessage);
 
 		// wrap in the TX proxy if necessary
 		if (!CollectionUtils.isEmpty(this.adviceChain)) {
@@ -229,6 +198,38 @@ private Runnable createMessageSendingTask(final Object mailMessage) {
 		return sendingTask;
 	}
 
+	private Runnable prepareSendingTask(Object mailMessage) {
+		return () -> {
+			@SuppressWarnings("unchecked")
+			org.springframework.messaging.Message<?> message =
+					mailMessage instanceof Message
+							? getMessageBuilderFactory().withPayload(mailMessage).build()
+							: (org.springframework.messaging.Message<Object>) mailMessage;
+
+			if (TransactionSynchronizationManager.isActualTransactionActive()
+					&& this.transactionSynchronizationFactory != null) {
+
+				TransactionSynchronization synchronization = this.transactionSynchronizationFactory.create(this);
+				if (synchronization != null) {
+					TransactionSynchronizationManager.registerSynchronization(synchronization);
+
+					if (synchronization instanceof IntegrationResourceHolderSynchronization
+							&& !TransactionSynchronizationManager.hasResource(this)) {
+
+						TransactionSynchronizationManager.bindResource(this,
+								((IntegrationResourceHolderSynchronization) synchronization).getResourceHolder());
+					}
+
+					Object resourceHolder = TransactionSynchronizationManager.getResource(this);
+					if (resourceHolder instanceof IntegrationResourceHolder) {
+						((IntegrationResourceHolder) resourceHolder).setMessage(message);
+					}
+				}
+			}
+			sendMessage(message);
+		};
+	}
+
 	private void publishException(Exception e) {
 		if (this.applicationEventPublisher != null) {
 			this.applicationEventPublisher.publishEvent(new ImapIdleExceptionEvent(e));
@@ -262,6 +263,7 @@ public void run() {
 				publishException(e);
 			}
 		}
+
 	}
 
 

spring-integration-mail/src/main/java/org/springframework/integration/mail/ImapMailReceiver.java

@@ -59,6 +59,8 @@ public class ImapMailReceiver extends AbstractMailReceiver {
 
 	private static final int DEFAULT_CANCEL_IDLE_INTERVAL = 120000;
 
+	private static final String PROTOCOL = "imap";
+
 	private final MessageCountListener messageCountListener = new SimpleMessageCountListener();
 
 	private final IdleCanceler idleCanceler = new IdleCanceler();
@@ -77,17 +79,17 @@ public class ImapMailReceiver extends AbstractMailReceiver {
 
 	public ImapMailReceiver() {
 		super();
-		this.setProtocol("imap");
+		setProtocol(PROTOCOL);
 	}
 
 	public ImapMailReceiver(String url) {
 		super(url);
 		if (url != null) {
-			Assert.isTrue(url.toLowerCase().startsWith("imap"),
+			Assert.isTrue(url.toLowerCase().startsWith(PROTOCOL),
 					"URL must start with 'imap' for the IMAP Mail receiver.");
 		}
 		else {
-			this.setProtocol("imap");
+			setProtocol(PROTOCOL);
 		}
 	}
 
@@ -126,7 +128,7 @@ public void setShouldMarkMessagesAsRead(Boolean shouldMarkMessagesAsRead) {
 	 * @since 3.0.5
 	 */
 	public void setCancelIdleInterval(long cancelIdleInterval) {
-		this.cancelIdleInterval = cancelIdleInterval * 1000;
+		this.cancelIdleInterval = cancelIdleInterval * 1000; // NOSONAR - convert seconds to milliseconds
 	}
 
 	@Override
@@ -140,7 +142,7 @@ protected void onInit() {
 			this.isInternalScheduler = true;
 		}
 		Properties javaMailProperties = getJavaMailProperties();
-		for (String name : new String[]{"imap", "imaps"}) {
+		for (String name : new String[] { PROTOCOL, "imaps" }) {
 			String peek = "mail." + name + ".peek";
 			if (javaMailProperties.getProperty(peek) == null) {
 				javaMailProperties.setProperty(peek, "true");
@@ -154,6 +156,9 @@ public void destroy() {
 		if (this.isInternalScheduler) {
 			((ThreadPoolTaskScheduler) this.scheduler).shutdown();
 		}
+		if (this.pingTask != null) {
+			this.pingTask.cancel(true);
+		}
 	}
 
 	/**
@@ -162,18 +167,16 @@ public void destroy() {
 	 * @throws MessagingException Any MessagingException.
 	 */
 	public void waitForNewMessages() throws MessagingException {
-		this.openFolder();
-		Folder folder = this.getFolder();
+		openFolder();
+		Folder folder = getFolder();
 		Assert.state(folder instanceof IMAPFolder,
-				"folder is not an instance of [" + IMAPFolder.class.getName() + "]");
+				() -> "folder is not an instance of [" + IMAPFolder.class.getName() + "]");
 		IMAPFolder imapFolder = (IMAPFolder) folder;
 		if (imapFolder.hasNewMessages()) {
 			return;
 		}
-		else if (!folder.getPermanentFlags().contains(Flags.Flag.RECENT)) {
-			if (searchForNewMessages().length > 0) {
-				return;
-			}
+		else if (!folder.getPermanentFlags().contains(Flags.Flag.RECENT) && searchForNewMessages().length > 0) {
+			return;
 		}
 		imapFolder.addMessageCountListener(this.messageCountListener);
 		try {
@@ -203,11 +206,11 @@ else if (!folder.getPermanentFlags().contains(Flags.Flag.RECENT)) {
 	 */
 	@Override
 	protected Message[] searchForNewMessages() throws MessagingException {
-		Flags supportedFlags = this.getFolder().getPermanentFlags();
-		SearchTerm searchTerm = this.compileSearchTerms(supportedFlags);
-		Folder folder = this.getFolder();
-		if (folder.isOpen()) {
-			return nullSafeMessages(searchTerm != null ? folder.search(searchTerm) : folder.getMessages());
+		Folder folderToUse = getFolder();
+		Flags supportedFlags = folderToUse.getPermanentFlags();
+		SearchTerm searchTerm = compileSearchTerms(supportedFlags);
+		if (folderToUse.isOpen()) {
+			return nullSafeMessages(searchTerm != null ? folderToUse.search(searchTerm) : folderToUse.getMessages());
 		}
 		throw new MessagingException("Folder is closed");
 	}
@@ -257,9 +260,11 @@ public void run() {
 					folder.isOpen(); // resets idle state
 				}
 			}
-			catch (Exception ignore) {
+			catch (Exception ex) {
+				logger.error("Error during resetting idle state.", ex);
 			}
 		}
+
 	}
 
 	/**
@@ -327,28 +332,35 @@ public SearchTerm generateSearchTerm(Flags supportedFlags, Folder folder) {
 			}
 
 			if (!recentFlagSupported) {
-				NotTerm notFlagged = null;
-				if (folder.getPermanentFlags().contains(Flags.Flag.USER)) {
+				searchTerm = applyTermsWhenNoRecentFlag(folder, searchTerm);
+			}
+			return searchTerm;
+		}
+
+		private SearchTerm applyTermsWhenNoRecentFlag(Folder folder, SearchTerm searchTerm) {
+			NotTerm notFlagged = null;
+			if (folder.getPermanentFlags().contains(Flag.USER)) {
+				if (logger.isDebugEnabled()) {
 					logger.debug("This email server does not support RECENT flag, but it does support " +
 							"USER flags which will be used to prevent duplicates during email fetch." +
 							" This receiver instance uses flag: " + getUserFlag());
-					Flags siFlags = new Flags();
-					siFlags.add(getUserFlag());
-					notFlagged = new NotTerm(new FlagTerm(siFlags, true));
-				}
-				else {
-					logger.debug("This email server does not support RECENT or USER flags. " +
-							"System flag 'Flag.FLAGGED' will be used to prevent duplicates during email fetch.");
-					notFlagged = new NotTerm(new FlagTerm(new Flags(Flags.Flag.FLAGGED), true));
-				}
-				if (searchTerm == null) {
-					searchTerm = notFlagged;
-				}
-				else {
-					searchTerm = new AndTerm(searchTerm, notFlagged);
 				}
+				Flags siFlags = new Flags();
+				siFlags.add(getUserFlag());
+				notFlagged = new NotTerm(new FlagTerm(siFlags, true));
+			}
+			else {
+				logger.debug("This email server does not support RECENT or USER flags. " +
+						"System flag 'Flag.FLAGGED' will be used to prevent duplicates during email fetch.");
+				notFlagged = new NotTerm(new FlagTerm(new Flags(Flag.FLAGGED), true));
+			}
+
+			if (searchTerm == null) {
+				return notFlagged;
+			}
+			else {
+				return new AndTerm(searchTerm, notFlagged);
 			}
-			return searchTerm;
 		}
 
 	}