Handle NullPointerException when sending email alerts (#5649)

* Handle NullPointerException when sending email

* Improve the log message related to the recipient list is empty

Co-authored-by: José Flauzino <jose@scclouds.com.br>

framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaAlertManagerImpl.java

@@ -53,6 +53,7 @@ import java.util.Set;
 import org.apache.cloudstack.utils.mailing.MailAddress;
 import org.apache.cloudstack.utils.mailing.SMTPMailProperties;
 import org.apache.cloudstack.utils.mailing.SMTPMailSender;
+import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.lang3.BooleanUtils;
 
 @Component
@@ -222,7 +223,7 @@ public class QuotaAlertManagerImpl extends ManagerBase implements QuotaAlertMana
             final String subject = templateEngine.replace(emailTemplate.getTemplateSubject());
             final String body = templateEngine.replace(emailTemplate.getTemplateBody());
             try {
-                sendQuotaAlert(emailRecipients, subject, body);
+                sendQuotaAlert(account.getUuid(), emailRecipients, subject, body);
                 emailToBeSent.sentSuccessfully(_quotaAcc);
             } catch (Exception e) {
                 s_logger.error(String.format("Unable to send quota alert email (subject=%s; body=%s) to account %s (%s) recipients (%s) due to error (%s)", subject, body, account.getAccountName(),
@@ -325,7 +326,7 @@ public class QuotaAlertManagerImpl extends ManagerBase implements QuotaAlertMana
         }
     };
 
-    protected void sendQuotaAlert(List<String> emails, String subject, String body) {
+    protected void sendQuotaAlert(String accountUuid, List<String> emails, String subject, String body) {
         SMTPMailProperties mailProperties = new SMTPMailProperties();
 
         mailProperties.setSender(new MailAddress(senderAddress));
@@ -333,6 +334,12 @@ public class QuotaAlertManagerImpl extends ManagerBase implements QuotaAlertMana
         mailProperties.setContent(body);
         mailProperties.setContentType("text/html; charset=utf-8");
 
+        if (CollectionUtils.isEmpty(emails)) {
+            s_logger.warn(String.format("Account [%s] does not have users with email registered, "
+                    + "therefore we are unable to send quota alert email with subject [%s] and content [%s].", accountUuid, subject, body));
+            return;
+        }
+
         Set<MailAddress> addresses = new HashSet<>();
         for (String email : emails) {
             addresses.add(new MailAddress(email));

framework/quota/src/test/java/org/apache/cloudstack/quota/QuotaAlertManagerImplTest.java

@@ -157,11 +157,12 @@ public class QuotaAlertManagerImplTest extends TestCase {
         Mockito.when(userDao.listByAccount(Mockito.anyLong())).thenReturn(users);
 
         quotaAlertManager.mailSender = Mockito.mock(SMTPMailSender.class);
-        Mockito.when(quotaAlertManager.mailSender.sendMail(Mockito.anyObject())).thenReturn(Boolean.TRUE);
+        Mockito.doNothing().when(quotaAlertManager.mailSender).sendMail(Mockito.any());
 
         quotaAlertManager.sendQuotaAlert(email);
         assertTrue(email.getSendDate() != null);
-        Mockito.verify(quotaAlertManager, Mockito.times(1)).sendQuotaAlert(Mockito.anyListOf(String.class), Mockito.anyString(), Mockito.anyString());
+
+        Mockito.verify(quotaAlertManager, Mockito.times(1)).sendQuotaAlert(Mockito.anyString(), Mockito.anyListOf(String.class), Mockito.anyString(), Mockito.anyString());
         Mockito.verify(quotaAlertManager.mailSender, Mockito.times(1)).sendMail(Mockito.any(SMTPMailProperties.class));
     }
 

server/src/main/java/com/cloud/alert/AlertManagerImpl.java

@@ -78,10 +78,11 @@ import java.util.Set;
 import org.apache.cloudstack.utils.mailing.MailAddress;
 import org.apache.cloudstack.utils.mailing.SMTPMailProperties;
 import org.apache.cloudstack.utils.mailing.SMTPMailSender;
+import org.apache.commons.lang3.ArrayUtils;
 import org.apache.commons.lang3.math.NumberUtils;
 
 public class AlertManagerImpl extends ManagerBase implements AlertManager, Configurable {
-    private static final Logger s_logger = Logger.getLogger(AlertManagerImpl.class.getName());
+    protected Logger logger = Logger.getLogger(AlertManagerImpl.class.getName());
 
     private static final long INITIAL_CAPACITY_CHECK_DELAY = 30L * 1000L; // Thirty seconds expressed in milliseconds.
 
@@ -226,7 +227,7 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
         try {
             clearAlert(alertType.getType(), dataCenterId, podId);
         } catch (Exception ex) {
-            s_logger.error("Problem clearing email alert", ex);
+            logger.error("Problem clearing email alert", ex);
         }
     }
 
@@ -242,11 +243,11 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
             if (mailSender != null) {
                 sendAlert(alertType, dataCenterId, podId, null, subject, body);
             } else {
-                s_logger.warn("AlertType:: " + alertType + " | dataCenterId:: " + dataCenterId + " | podId:: " + podId +
+                logger.warn("AlertType:: " + alertType + " | dataCenterId:: " + dataCenterId + " | podId:: " + podId +
                         " | message:: " + subject + " | body:: " + body);
             }
         } catch (Exception ex) {
-            s_logger.error("Problem sending email alert", ex);
+            logger.error("Problem sending email alert", ex);
         }
     }
 
@@ -261,9 +262,9 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
 
         try {
 
-            if (s_logger.isDebugEnabled()) {
+            if (logger.isDebugEnabled()) {
-                s_logger.debug("recalculating system capacity");
+                logger.debug("recalculating system capacity");
-                s_logger.debug("Executing cpu/ram capacity update");
+                logger.debug("Executing cpu/ram capacity update");
             }
 
             // Calculate CPU and RAM capacities
@@ -280,9 +281,9 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
                     _capacityMgr.updateCapacityForHost(host, offeringsMap);
                 }
             }
-            if (s_logger.isDebugEnabled()) {
+            if (logger.isDebugEnabled()) {
-                s_logger.debug("Done executing cpu/ram capacity update");
+                logger.debug("Done executing cpu/ram capacity update");
-                s_logger.debug("Executing storage capacity update");
+                logger.debug("Executing storage capacity update");
             }
             // Calculate storage pool capacity
             List<StoragePoolVO> storagePools = _storagePoolDao.listAll();
@@ -295,9 +296,9 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
                 }
             }
 
-            if (s_logger.isDebugEnabled()) {
+            if (logger.isDebugEnabled()) {
-                s_logger.debug("Done executing storage capacity update");
+                logger.debug("Done executing storage capacity update");
-                s_logger.debug("Executing capacity updates for public ip and Vlans");
+                logger.debug("Executing capacity updates for public ip and Vlans");
             }
 
             List<DataCenterVO> datacenters = _dcDao.listAll();
@@ -324,9 +325,9 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
                 }
             }
 
-            if (s_logger.isDebugEnabled()) {
+            if (logger.isDebugEnabled()) {
-                s_logger.debug("Done capacity updates for public ip and Vlans");
+                logger.debug("Done capacity updates for public ip and Vlans");
-                s_logger.debug("Executing capacity updates for private ip");
+                logger.debug("Executing capacity updates for private ip");
             }
 
             // Calculate new Private IP capacity
@@ -338,13 +339,13 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
                 createOrUpdateIpCapacity(dcId, podId, Capacity.CAPACITY_TYPE_PRIVATE_IP, _configMgr.findPodAllocationState(pod));
             }
 
-            if (s_logger.isDebugEnabled()) {
+            if (logger.isDebugEnabled()) {
-                s_logger.debug("Done executing capacity updates for private ip");
+                logger.debug("Done executing capacity updates for private ip");
-                s_logger.debug("Done recalculating system capacity");
+                logger.debug("Done recalculating system capacity");
             }
 
         } catch (Throwable t) {
-            s_logger.error("Caught exception in recalculating capacity", t);
+            logger.error("Caught exception in recalculating capacity", t);
         }
     }
 
@@ -419,11 +420,11 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
         @Override
         protected void runInContext() {
             try {
-                s_logger.debug("Running Capacity Checker ... ");
+                logger.debug("Running Capacity Checker ... ");
                 checkForAlerts();
-                s_logger.debug("Done running Capacity Checker ... ");
+                logger.debug("Done running Capacity Checker ... ");
             } catch (Throwable t) {
-                s_logger.error("Exception in CapacityChecker", t);
+                logger.error("Exception in CapacityChecker", t);
             }
         }
     }
@@ -628,13 +629,13 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
         }
 
         try {
-            if (s_logger.isDebugEnabled()) {
+            if (logger.isDebugEnabled()) {
-                s_logger.debug(msgSubject);
+                logger.debug(msgSubject);
-                s_logger.debug(msgContent);
+                logger.debug(msgContent);
             }
             sendAlert(alertType, dc.getId(), podId, clusterId, msgSubject, msgContent);
         } catch (Exception ex) {
-            s_logger.error("Exception in CapacityChecker", ex);
+            logger.error("Exception in CapacityChecker", ex);
         }
     }
 
@@ -682,7 +683,7 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
 
     public void sendAlert(AlertType alertType, long dataCenterId, Long podId, Long clusterId, String subject, String content)
             throws MessagingException, UnsupportedEncodingException {
-        s_logger.warn(String.format("alertType=[%s] dataCenterId=[%s] podId=[%s] clusterId=[%s] message=[%s].", alertType, dataCenterId, podId, clusterId, subject));
+        logger.warn(String.format("alertType=[%s] dataCenterId=[%s] podId=[%s] clusterId=[%s] message=[%s].", alertType, dataCenterId, podId, clusterId, subject));
         AlertVO alert = null;
         if ((alertType != AlertManager.AlertType.ALERT_TYPE_HOST) && (alertType != AlertManager.AlertType.ALERT_TYPE_USERVM)
                 && (alertType != AlertManager.AlertType.ALERT_TYPE_DOMAIN_ROUTER) && (alertType != AlertManager.AlertType.ALERT_TYPE_CONSOLE_PROXY)
@@ -706,14 +707,13 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
             newAlert.setName(alertType.getName());
             _alertDao.persist(newAlert);
         } else {
-            if (s_logger.isDebugEnabled()) {
+            logger.debug("Have already sent: " + alert.getSentCount() + " emails for alert type '" + alertType + "' -- skipping send email");
-                    s_logger.debug("Have already sent: " + alert.getSentCount() + " emails for alert type '" + alertType + "' -- skipping send email");
-                }
             return;
         }
 
-        if (recipients == null) {
+        if (ArrayUtils.isEmpty(recipients)) {
-            s_logger.warn(String.format("No recipients set in 'alert.email.addresses', skipping sending alert with subject: %s and content: %s", subject, content));
+            logger.warn(String.format("No recipients set in global setting 'alert.email.addresses', "
+                    + "skipping sending alert with subject [%s] and content [%s].", subject, content));
             return;
         }
 
@@ -734,7 +734,7 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
 
     }
 
-    private void sendMessage(SMTPMailProperties mailProps) {
+    protected void sendMessage(SMTPMailProperties mailProps) {
         _executor.execute(new Runnable() {
             @Override
             public void run() {
@@ -770,7 +770,7 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
             sendAlert(alertType, dataCenterId, podId, msg, msg);
             return true;
         } catch (Exception ex) {
-            s_logger.warn("Failed to generate an alert of type=" + alertType + "; msg=" + msg);
+            logger.warn("Failed to generate an alert of type=" + alertType + "; msg=" + msg);
             return false;
         }
     }

server/src/test/java/com/cloud/alert/AlertManagerImplTest.java

@@ -0,0 +1,99 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// the License.  You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package com.cloud.alert;
+
+import java.io.UnsupportedEncodingException;
+
+import javax.mail.MessagingException;
+
+import org.apache.cloudstack.utils.mailing.SMTPMailSender;
+import org.apache.log4j.Logger;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.alert.dao.AlertDao;
+
+@RunWith(MockitoJUnitRunner.class)
+public class AlertManagerImplTest {
+
+    @Spy
+    @InjectMocks
+    AlertManagerImpl alertManagerImplMock;
+
+    @Mock
+    AlertDao alertDaoMock;
+
+    @Mock
+    AlertVO alertVOMock;
+
+    @Mock
+    Logger loggerMock;
+
+    @Mock
+    SMTPMailSender mailSenderMock;
+
+    private void sendMessage (){
+        try {
+            alertManagerImplMock.sendAlert(AlertManager.AlertType.ALERT_TYPE_CPU, 0, 1l, 1l, "", "");
+        } catch (UnsupportedEncodingException | MessagingException e) {
+            Assert.fail();
+        }
+    }
+
+    @Test
+    public void sendAlertTestSendMail() {
+        Mockito.doReturn(null).when(alertDaoMock).getLastAlert(Mockito.anyShort(), Mockito.anyLong(),
+                Mockito.anyLong(), Mockito.anyLong());
+        Mockito.doReturn(null).when(alertDaoMock).persist(Mockito.any());
+        alertManagerImplMock.recipients = new String [] {""};
+
+        sendMessage();
+
+        Mockito.verify(alertManagerImplMock).sendMessage(Mockito.any());
+    }
+
+    @Test
+    public void sendAlertTestDebugLogging() {
+        Mockito.doReturn(0).when(alertVOMock).getSentCount();
+        Mockito.doReturn(alertVOMock).when(alertDaoMock).getLastAlert(Mockito.anyShort(), Mockito.anyLong(),
+                Mockito.anyLong(), Mockito.anyLong());
+
+        sendMessage();
+
+        Mockito.verify(alertManagerImplMock.logger).debug(Mockito.anyString());
+        Mockito.verify(alertManagerImplMock, Mockito.never()).sendMessage(Mockito.any());
+    }
+
+    @Test
+    public void sendAlertTestWarnLogging() {
+        Mockito.doReturn(null).when(alertDaoMock).getLastAlert(Mockito.anyShort(), Mockito.anyLong(),
+                Mockito.anyLong(), Mockito.anyLong());
+        Mockito.doReturn(null).when(alertDaoMock).persist(Mockito.any());
+        alertManagerImplMock.recipients = null;
+
+        sendMessage();
+
+        Mockito.verify(alertManagerImplMock.logger, Mockito.times(2)).warn(Mockito.anyString());
+        Mockito.verify(alertManagerImplMock, Mockito.never()).sendMessage(Mockito.any());
+    }
+}

usage/src/main/java/com/cloud/usage/UsageAlertManagerImpl.java

@@ -35,17 +35,18 @@ import java.util.Set;
 import org.apache.cloudstack.utils.mailing.MailAddress;
 import org.apache.cloudstack.utils.mailing.SMTPMailProperties;
 import org.apache.cloudstack.utils.mailing.SMTPMailSender;
+import org.apache.commons.lang3.ArrayUtils;
 
 @Component
 public class UsageAlertManagerImpl extends ManagerBase implements AlertManager {
-    private static final Logger s_logger = Logger.getLogger(UsageAlertManagerImpl.class.getName());
+    protected Logger logger = Logger.getLogger(UsageAlertManagerImpl.class.getName());
 
     private String senderAddress;
     protected SMTPMailSender mailSender;
     protected String[] recipients;
 
     @Inject
-    private AlertDao _alertDao;
+    protected AlertDao _alertDao;
     @Inject
     private ConfigurationDao _configDao;
 
@@ -72,7 +73,7 @@ public class UsageAlertManagerImpl extends ManagerBase implements AlertManager {
         try {
             clearAlert(alertType.getType(), dataCenterId, podId);
         } catch (Exception ex) {
-            s_logger.error("Problem clearing email alert", ex);
+            logger.error("Problem clearing email alert", ex);
         }
     }
 
@@ -96,7 +97,7 @@ public class UsageAlertManagerImpl extends ManagerBase implements AlertManager {
             newAlert.setName(alertType.getName());
             _alertDao.persist(newAlert);
         } else {
-            s_logger.debug(String.format("Have already sent: [%s] emails for alert type [%s] -- skipping send email.", alert.getSentCount(), alertType));
+            logger.debug(String.format("Have already sent [%s] emails for alert type [%s] -- skipping send email.", alert.getSentCount(), alertType));
             return;
         }
 
@@ -106,6 +107,12 @@ public class UsageAlertManagerImpl extends ManagerBase implements AlertManager {
         mailProps.setContent(content);
         mailProps.setContentType("text/plain");
 
+        if (ArrayUtils.isEmpty(recipients)) {
+            logger.warn(String.format("No recipients set in global setting 'alert.email.addresses', "
+                    + "skipping sending alert with subject [%s] and content [%s].", subject, content));
+            return;
+        }
+
         Set<MailAddress> addresses = new HashSet<>();
         for (String recipient : recipients) {
             addresses.add(new MailAddress(recipient));
@@ -138,7 +145,7 @@ public class UsageAlertManagerImpl extends ManagerBase implements AlertManager {
             sendAlert(alertType, dataCenterId, podId, msg, msg);
             return true;
         } catch (Exception ex) {
-            s_logger.warn("Failed to generate an alert of type=" + alertType + "; msg=" + msg, ex);
+            logger.warn("Failed to generate an alert of type=" + alertType + "; msg=" + msg, ex);
             return false;
         }
     }

usage/src/test/java/com/cloud/usage/UsageAlertManagerImplTest.java

@@ -0,0 +1,85 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// the License.  You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package com.cloud.usage;
+
+import org.apache.cloudstack.utils.mailing.SMTPMailSender;
+import org.apache.log4j.Logger;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.alert.AlertManager;
+import com.cloud.alert.AlertVO;
+import com.cloud.alert.dao.AlertDao;
+
+@RunWith(MockitoJUnitRunner.class)
+public class UsageAlertManagerImplTest {
+
+    @Spy
+    @InjectMocks
+    UsageAlertManagerImpl usageAlertManagerImplMock;
+
+    @Mock
+    AlertDao alertDaoMock;
+
+    @Mock
+    AlertVO alertVOMock;
+
+    @Mock
+    Logger loggerMock;
+
+    @Mock
+    SMTPMailSender mailSenderMock;
+
+    @Test
+    public void sendAlertTestSendMail() {
+        Mockito.doReturn(null).when(alertDaoMock).getLastAlert(Mockito.anyShort(), Mockito.anyLong(), Mockito.anyLong());
+        Mockito.doReturn(null).when(alertDaoMock).persist(Mockito.any());
+        usageAlertManagerImplMock.recipients = new String [] {""};
+
+        usageAlertManagerImplMock.sendAlert(AlertManager.AlertType.ALERT_TYPE_CPU, 0, 1l, "", "");
+
+        Mockito.verify(usageAlertManagerImplMock.mailSender).sendMail(Mockito.any());
+    }
+
+    @Test
+    public void sendAlertTestDebugLogging() {
+        Mockito.doReturn(0).when(alertVOMock).getSentCount();
+        Mockito.doReturn(alertVOMock).when(alertDaoMock).getLastAlert(Mockito.anyShort(), Mockito.anyLong(), Mockito.anyLong());
+
+        usageAlertManagerImplMock.sendAlert(AlertManager.AlertType.ALERT_TYPE_CPU, 0, 1l, "", "");
+
+        Mockito.verify(usageAlertManagerImplMock.logger).debug(Mockito.anyString());
+        Mockito.verify(usageAlertManagerImplMock.mailSender, Mockito.never()).sendMail(Mockito.any());
+    }
+
+    @Test
+    public void sendAlertTestWarnLogging() {
+        Mockito.doReturn(null).when(alertDaoMock).getLastAlert(Mockito.anyShort(), Mockito.anyLong(), Mockito.anyLong());
+        Mockito.doReturn(null).when(alertDaoMock).persist(Mockito.any());
+        usageAlertManagerImplMock.recipients = null;
+
+        usageAlertManagerImplMock.sendAlert(AlertManager.AlertType.ALERT_TYPE_CPU, 0, 1l, "", "");
+
+        Mockito.verify(usageAlertManagerImplMock.logger).warn(Mockito.anyString());
+        Mockito.verify(usageAlertManagerImplMock.mailSender, Mockito.never()).sendMail(Mockito.any());
+    }
+}

utils/src/main/java/org/apache/cloudstack/utils/mailing/SMTPMailSender.java

@@ -31,6 +31,9 @@ import javax.mail.PasswordAuthentication;
 import javax.mail.Session;
 import javax.mail.URLName;
 import javax.mail.internet.InternetAddress;
+
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.commons.lang3.ArrayUtils;
 import org.apache.commons.lang3.BooleanUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.math.NumberUtils;
@@ -39,7 +42,7 @@ import org.apache.log4j.Logger;
 
 public class SMTPMailSender {
 
-    private Logger logger = Logger.getLogger(SMTPMailSender.class);
+    protected Logger logger = Logger.getLogger(SMTPMailSender.class);
 
     protected Session session = null;
     protected SMTPSessionProperties sessionProps;
@@ -151,28 +154,30 @@ public class SMTPMailSender {
         return sessionProps;
     }
 
-    public boolean sendMail(SMTPMailProperties mailProps) {
+    public void sendMail(SMTPMailProperties mailProps) {
         if (session == null) {
             logger.error("Unable to send mail due to null session.");
-            return false;
+            return;
         }
 
         try {
             SMTPMessage message = createMessage(mailProps);
 
+            if (ArrayUtils.isEmpty(message.getAllRecipients())) {
+                logger.error(String.format("Unable to send mail [%s] due to the recipient list of the message being empty.",
+                        mailProps.getSubject()));
+                return;
+            }
+
             SMTPTransport smtpTrans = createSmtpTransport();
 
             smtpTrans.connect();
             smtpTrans.sendMessage(message, message.getAllRecipients());
             smtpTrans.close();
 
-            return true;
         } catch (MessagingException | UnsupportedEncodingException ex) {
             logger.error(String.format("Unable to send mail [%s] to the recipcients [%s].", mailProps.getSubject(), mailProps.getRecipients().toString()), ex);
         }
-
-        return false;
-
     }
 
     protected SMTPMessage createMessage(SMTPMailProperties mailProps) throws MessagingException, UnsupportedEncodingException {
@@ -207,7 +212,12 @@ public class SMTPMailSender {
         return new SMTPTransport(session, urlName);
     }
 
-    protected boolean setMailRecipients(SMTPMessage message, Set<MailAddress> recipients, String subject) throws UnsupportedEncodingException, MessagingException {
+    protected void setMailRecipients(SMTPMessage message, Set<MailAddress> recipients, String subject) throws UnsupportedEncodingException, MessagingException {
+        if (CollectionUtils.isEmpty(recipients)) {
+            logger.error("Unable to set recipients due to the recipient list being empty.");
+            return;
+        }
+
         for (MailAddress recipient : recipients) {
             if (StringUtils.isNotBlank(recipient.getAddress())) {
                 try {
@@ -218,14 +228,6 @@ public class SMTPMailSender {
                 }
             }
         }
-
-        if (recipients.isEmpty() || message.getAllRecipients().length == 0) {
-            logger.error("Unable to send mail due to empty list of recipients.");
-            logger.debug(String.format("Unable to send message [%s].", subject));
-            return false;
-        }
-
-        return true;
     }
 
 }

utils/src/test/java/org/apache/cloudstack/utils/mailing/SMTPMailSenderTest.java

@@ -28,20 +28,36 @@ import java.util.Set;
 import javax.mail.Address;
 import javax.mail.MessagingException;
 import javax.mail.Session;
+import javax.mail.internet.InternetAddress;
 import javax.mail.internet.MimeMessage;
 import junit.framework.TestCase;
 import org.apache.commons.lang3.time.DateUtils;
 import org.apache.commons.mail.EmailConstants;
+import org.apache.log4j.Logger;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
 import org.mockito.Mockito;
+import org.mockito.Spy;
 import org.mockito.runners.MockitoJUnitRunner;
 
 @RunWith(MockitoJUnitRunner.class)
 public class SMTPMailSenderTest extends TestCase {
 
     private SMTPMailSender smtpMailSender;
+
+    @Spy
+    @InjectMocks
+    private SMTPMailSender smtpMailSenderMock;
+
+    @Mock
+    Logger loggerMock;
+
+    @Mock
+    SMTPMessage messageMock;
+
     private Map<String, String> configsMock = Mockito.mock(Map.class);
     private String namespace = "test";
     private String enabledProtocols = "mail.smtp.ssl.protocols";
@@ -447,8 +463,10 @@ public class SMTPMailSenderTest extends TestCase {
         mailProps.setSender(new MailAddress("test@test.com"));
         mailProps.setContent("A simple test");
         mailProps.setContentType("text/plain");
+        mailProps.setRecipients(new HashSet<>());
+        mailProps.setSubject("Test");
 
-        Mockito.doReturn(true).when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.any(), Mockito.any());
+        Mockito.doNothing().when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.anySet(), Mockito.anyString());
 
         SMTPMessage message = smtpMailSender.createMessage(mailProps);
 
@@ -465,8 +483,10 @@ public class SMTPMailSenderTest extends TestCase {
         mailProps.setFrom(new MailAddress("test2@test2.com", "TEST2"));
         mailProps.setContent("A simple test");
         mailProps.setContentType("text/plain");
+        mailProps.setRecipients(new HashSet<>());
+        mailProps.setSubject("Test");
 
-        Mockito.doReturn(true).when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.any(), Mockito.any());
+        Mockito.doNothing().when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.anySet(), Mockito.anyString());
 
         SMTPMessage message = smtpMailSender.createMessage(mailProps);
 
@@ -490,8 +510,10 @@ public class SMTPMailSenderTest extends TestCase {
         cal.set(Calendar.DAY_OF_MONTH, 1);
 
         mailProps.setSentDate(cal.getTime());
+        mailProps.setRecipients(new HashSet<>());
+        mailProps.setSubject("Test");
 
-        Mockito.doReturn(true).when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.any(), Mockito.any());
+        Mockito.doNothing().when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.anySet(), Mockito.anyString());
 
         SMTPMessage message = smtpMailSender.createMessage(mailProps);
         assertTrue(DateUtils.truncatedEquals(cal.getTime(), message.getSentDate(), Calendar.SECOND));
@@ -511,8 +533,9 @@ public class SMTPMailSenderTest extends TestCase {
         mailProps.setSubject(subject);
         mailProps.setContent(content);
         mailProps.setContentType(contentType);
+        mailProps.setRecipients(new HashSet<>());
 
-        Mockito.doReturn(true).when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.any(), Mockito.any());
+        Mockito.doNothing().when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.anySet(), Mockito.anyString());
 
         SMTPMessage message = smtpMailSender.createMessage(mailProps);
         assertEquals(subject, message.getSubject());
@@ -531,7 +554,7 @@ public class SMTPMailSenderTest extends TestCase {
         recipients.add(new MailAddress("smtp.acme.org"));
         recipients.add(new MailAddress("smtp.acme2.org", "Coyote"));
 
-        boolean returnOfSetEmail = smtpMailSender.setMailRecipients(messageMock, recipients, "A simple test");
+        smtpMailSender.setMailRecipients(messageMock, recipients, "A simple test");
 
         Address[] allRecipients = messageMock.getAllRecipients();
 
@@ -540,8 +563,6 @@ public class SMTPMailSenderTest extends TestCase {
 
         assertEquals("\"smtp.acme.org\" <smtp.acme.org>", allRecipients[0].toString());
         assertEquals("Coyote <smtp.acme2.org>", allRecipients[1].toString());
-
-        assertTrue(returnOfSetEmail);
     }
 
     @Test
@@ -556,38 +577,45 @@ public class SMTPMailSenderTest extends TestCase {
         recipients.add(new MailAddress(""));
         recipients.add(new MailAddress("  "));
 
-        boolean returnOfSetEmail = smtpMailSender.setMailRecipients(messageMock, recipients, "A simple test");
+        smtpMailSender.setMailRecipients(messageMock, recipients, "A simple test");
 
         Address[] allRecipients = messageMock.getAllRecipients();
 
         int expectedNumberOfValidRecipientsConfigured = 0;
         assertEquals(expectedNumberOfValidRecipientsConfigured, allRecipients.length);
+    }
 
-        assertFalse(returnOfSetEmail);
+    @Test
+    public void setMailRecipientsTestWarnLogging() throws UnsupportedEncodingException, MessagingException  {
+        SMTPMessage messageMock = new SMTPMessage(Mockito.mock(MimeMessage.class));
+
+        smtpMailSenderMock.setMailRecipients(messageMock, null, "");
+
+        Mockito.verify(smtpMailSenderMock.logger).error(Mockito.anyString());
     }
 
     @Test
     public void validateSMTPMailSenderSendMailWithNullSession() {
         SMTPMailProperties mailProps = new SMTPMailProperties();
 
-        boolean returnOfSendMail = smtpMailSender.sendMail(mailProps);
+        smtpMailSenderMock.sendMail(mailProps);
 
-        assertFalse(returnOfSendMail);
+        Mockito.verify(smtpMailSenderMock.logger).error(Mockito.anyString());
     }
 
     @Test
     public void validateSMTPMailSenderSendMailWithValidSession() throws MessagingException, UnsupportedEncodingException {
-        smtpMailSender = smtpMailSender = Mockito.spy(smtpMailSender);
         SMTPMailProperties mailProps = new SMTPMailProperties();
+        smtpMailSenderMock.session = Session.getDefaultInstance(Mockito.mock(Properties.class));
+        Address[] recipients = {new InternetAddress("email@acme.com")};
 
-        smtpMailSender.session = Session.getDefaultInstance(Mockito.mock(Properties.class));
+        Mockito.doReturn(recipients).when(messageMock).getAllRecipients();
+        Mockito.doReturn(messageMock).when(smtpMailSenderMock).createMessage(Mockito.any());
+        Mockito.doReturn(Mockito.mock(SMTPTransport.class)).when(smtpMailSenderMock).createSmtpTransport();
 
-        Mockito.doReturn(Mockito.mock(SMTPMessage.class)).when(smtpMailSender).createMessage(Mockito.any(SMTPMailProperties.class));
+        smtpMailSenderMock.sendMail(mailProps);
-        Mockito.doReturn(Mockito.mock(SMTPTransport.class)).when(smtpMailSender).createSmtpTransport();
 
-        boolean returnOfSendMail = smtpMailSender.sendMail(mailProps);
+        Mockito.verify(smtpMailSenderMock.logger, Mockito.never()).error(Mockito.anyString());
-
-        assertTrue(returnOfSendMail);
     }
 
     @Test