From dd9dd6bae080036ca72d27e2aeb8bfd98a4f5933 Mon Sep 17 00:00:00 2001 From: ivankozka Date: Fri, 1 Jul 2022 12:58:23 +0300 Subject: [PATCH 1/4] implement support for timeout in `DefaultMailService` --- .../service/mail/DefaultMailService.java | 49 +++++++++++++------ 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/application/src/main/java/org/thingsboard/server/service/mail/DefaultMailService.java b/application/src/main/java/org/thingsboard/server/service/mail/DefaultMailService.java index 7e1a3d7fa7..1997a2ffdd 100644 --- a/application/src/main/java/org/thingsboard/server/service/mail/DefaultMailService.java +++ b/application/src/main/java/org/thingsboard/server/service/mail/DefaultMailService.java @@ -16,10 +16,12 @@ package org.thingsboard.server.service.mail; import com.fasterxml.jackson.databind.JsonNode; +import com.google.common.util.concurrent.Futures; import freemarker.template.Configuration; import freemarker.template.Template; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.MessageSource; import org.springframework.context.annotation.Lazy; @@ -54,6 +56,8 @@ import java.util.HashMap; import java.util.Locale; import java.util.Map; import java.util.Properties; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; @Service @Slf4j @@ -70,6 +74,8 @@ public class DefaultMailService implements MailService { private final AdminSettingsService adminSettingsService; private final TbApiUsageClient apiUsageClient; + private static final long DEFAULT_TIMEOUT = 10_000; + @Lazy @Autowired private TbApiUsageStateService apiUsageStateService; @@ -81,6 +87,8 @@ public class DefaultMailService implements MailService { private String mailFrom; + private long timeout; + public DefaultMailService(MessageSource messages, Configuration freemarkerConfig, AdminSettingsService adminSettingsService, TbApiUsageClient apiUsageClient) { this.messages = messages; this.freemarkerConfig = freemarkerConfig; @@ -100,6 +108,7 @@ public class DefaultMailService implements MailService { JsonNode jsonConfig = settings.getJsonValue(); mailSender = createMailSender(jsonConfig); mailFrom = jsonConfig.get("mailFrom").asText(); + timeout = jsonConfig.get("timeout").asLong(DEFAULT_TIMEOUT); } else { throw new IncorrectParameterException("Failed to update mail configuration. Settings not found!"); } @@ -166,7 +175,7 @@ public class DefaultMailService implements MailService { @Override public void sendEmail(TenantId tenantId, String email, String subject, String message) throws ThingsboardException { - sendMail(mailSender, mailFrom, email, subject, message); + sendMail(mailSender, mailFrom, email, subject, message, timeout); } @Override @@ -174,13 +183,14 @@ public class DefaultMailService implements MailService { JavaMailSenderImpl testMailSender = createMailSender(jsonConfig); String mailFrom = jsonConfig.get("mailFrom").asText(); String subject = messages.getMessage("test.message.subject", null, Locale.US); + long timeout = jsonConfig.get("timeout").asLong(DEFAULT_TIMEOUT); Map model = new HashMap<>(); model.put(TARGET_EMAIL, email); String message = mergeTemplateIntoString("test.ftl", model); - sendMail(testMailSender, mailFrom, email, subject, message); + sendMail(testMailSender, mailFrom, email, subject, message, timeout); } @Override @@ -194,7 +204,7 @@ public class DefaultMailService implements MailService { String message = mergeTemplateIntoString("activation.ftl", model); - sendMail(mailSender, mailFrom, email, subject, message); + sendMail(mailSender, mailFrom, email, subject, message, timeout); } @Override @@ -208,7 +218,7 @@ public class DefaultMailService implements MailService { String message = mergeTemplateIntoString("account.activated.ftl", model); - sendMail(mailSender, mailFrom, email, subject, message); + sendMail(mailSender, mailFrom, email, subject, message, timeout); } @Override @@ -222,7 +232,7 @@ public class DefaultMailService implements MailService { String message = mergeTemplateIntoString("reset.password.ftl", model); - sendMail(mailSender, mailFrom, email, subject, message); + sendMail(mailSender, mailFrom, email, subject, message, timeout); } @Override @@ -247,7 +257,7 @@ public class DefaultMailService implements MailService { String message = mergeTemplateIntoString("password.was.reset.ftl", model); - sendMail(mailSender, mailFrom, email, subject, message); + sendMail(mailSender, mailFrom, email, subject, message, timeout); } @Override @@ -287,7 +297,7 @@ public class DefaultMailService implements MailService { helper.addInline(imgId, iss, contentType); } } - javaMailSender.send(helper.getMimeMessage()); + sendMailWithTimeout(javaMailSender, helper.getMimeMessage(), timeout); apiUsageClient.report(tenantId, customerId, ApiUsageRecordKey.EMAIL_EXEC_COUNT, 1); } catch (Exception e) { throw handleException(e); @@ -308,7 +318,7 @@ public class DefaultMailService implements MailService { String message = mergeTemplateIntoString("account.lockout.ftl", model); - sendMail(mailSender, mailFrom, email, subject, message); + sendMail(mailSender, mailFrom, email, subject, message, timeout); } @Override @@ -320,7 +330,7 @@ public class DefaultMailService implements MailService { "expirationTimeSeconds", expirationTimeSeconds )); - sendMail(mailSender, mailFrom, email, subject, message); + sendMail(mailSender, mailFrom, email, subject, message, timeout); } @Override @@ -347,7 +357,7 @@ public class DefaultMailService implements MailService { message = mergeTemplateIntoString("state.disabled.ftl", model); break; } - sendMail(mailSender, mailFrom, email, subject, message); + sendMail(mailSender, mailFrom, email, subject, message, timeout); } @Override @@ -447,9 +457,8 @@ public class DefaultMailService implements MailService { } } - private void sendMail(JavaMailSenderImpl mailSender, - String mailFrom, String email, - String subject, String message) throws ThingsboardException { + private void sendMail(JavaMailSenderImpl mailSender, String mailFrom, String email, + String subject, String message, long timeout) throws ThingsboardException { try { MimeMessage mimeMsg = mailSender.createMimeMessage(); MimeMessageHelper helper = new MimeMessageHelper(mimeMsg, UTF_8); @@ -457,12 +466,24 @@ public class DefaultMailService implements MailService { helper.setTo(email); helper.setSubject(subject); helper.setText(message, true); - mailSender.send(helper.getMimeMessage()); + + sendMailWithTimeout(mailSender, helper.getMimeMessage(), timeout); } catch (Exception e) { throw handleException(e); } } + private void sendMailWithTimeout(JavaMailSender mailSender, MimeMessage msg, long timeout) { + var submittedMail = Futures.submit(() -> mailSender.send(msg), mailExecutorService); + try { + submittedMail.get(timeout, TimeUnit.MILLISECONDS); + } catch (TimeoutException ignored) { + throw new RuntimeException("Timeout!"); + } catch (Exception e) { + throw new RuntimeException(ExceptionUtils.getRootCause(e)); + } + } + private String mergeTemplateIntoString(String templateLocation, Map model) throws ThingsboardException { try { From 8efb0dddab1586907b3de5042a9e31b0538eca63 Mon Sep 17 00:00:00 2001 From: ivankozka Date: Mon, 4 Jul 2022 13:15:04 +0300 Subject: [PATCH 2/4] add corresponding tests --- .../controller/BaseAdminControllerTest.java | 44 +++++++++++++++++-- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/application/src/test/java/org/thingsboard/server/controller/BaseAdminControllerTest.java b/application/src/test/java/org/thingsboard/server/controller/BaseAdminControllerTest.java index b89133f29a..81ecce8300 100644 --- a/application/src/test/java/org/thingsboard/server/controller/BaseAdminControllerTest.java +++ b/application/src/test/java/org/thingsboard/server/controller/BaseAdminControllerTest.java @@ -18,14 +18,29 @@ package org.thingsboard.server.controller; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.ObjectNode; import org.junit.Test; +import org.mockito.Mockito; +import org.springframework.beans.factory.annotation.Autowired; +import org.thingsboard.common.util.JacksonUtil; +import org.thingsboard.rule.engine.api.MailService; import org.thingsboard.server.common.data.AdminSettings; +import org.thingsboard.server.service.mail.DefaultMailService; -import static org.hamcrest.Matchers.*; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; public abstract class BaseAdminControllerTest extends AbstractControllerTest { + @Autowired + MailService mailService; + + @Autowired + DefaultMailService defaultMailService; + @Test public void testFindAdminSettingsByKey() throws Exception { loginSysAdmin(); @@ -100,5 +115,28 @@ public abstract class BaseAdminControllerTest extends AbstractControllerTest { doPost("/api/admin/settings/testMail", adminSettings) .andExpect(status().isOk()); } - + + @Test + public void testSendTestMailTimeout() throws Exception { + loginSysAdmin(); + AdminSettings adminSettings = doGet("/api/admin/settings/mail", AdminSettings.class); + ObjectNode objectNode = JacksonUtil.fromString(adminSettings.getJsonValue().toString(), ObjectNode.class); + + objectNode.put("smtpHost", "mail.gandi.net"); + objectNode.put("timeout", 1_000); + objectNode.put("username", "username"); + objectNode.put("password", "password"); + + adminSettings.setJsonValue(objectNode); + + Mockito.doAnswer((invocations) -> { + var jsonConfig = (JsonNode) invocations.getArgument(0); + var email = (String) invocations.getArgument(1); + + defaultMailService.sendTestMail(jsonConfig, email); + return null; + }).when(mailService).sendTestMail(Mockito.any(), Mockito.anyString()); + doPost("/api/admin/settings/testMail", adminSettings).andExpect(status().is5xxServerError()); + Mockito.doNothing().when(mailService).sendTestMail(Mockito.any(), Mockito.any()); + } } From 33b502e1569cc09b63f6de4228bae377cf9ac0c6 Mon Sep 17 00:00:00 2001 From: ivankozka Date: Wed, 6 Jul 2022 13:32:30 +0300 Subject: [PATCH 3/4] add timeoutExecutor to prevent deadlocks; fix not using timeout from configuration in TbSendEmailNode --- .../service/mail/DefaultMailService.java | 31 +++++++++++++++---- .../src/main/resources/thingsboard.yml | 4 ++- .../rule/engine/api/MailService.java | 3 +- .../rule/engine/mail/TbSendEmailNode.java | 2 +- 4 files changed, 30 insertions(+), 10 deletions(-) diff --git a/application/src/main/java/org/thingsboard/server/service/mail/DefaultMailService.java b/application/src/main/java/org/thingsboard/server/service/mail/DefaultMailService.java index 1997a2ffdd..8140c79189 100644 --- a/application/src/main/java/org/thingsboard/server/service/mail/DefaultMailService.java +++ b/application/src/main/java/org/thingsboard/server/service/mail/DefaultMailService.java @@ -23,6 +23,7 @@ import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.exception.ExceptionUtils; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; import org.springframework.context.MessageSource; import org.springframework.context.annotation.Lazy; import org.springframework.core.NestedRuntimeException; @@ -34,6 +35,7 @@ import org.springframework.stereotype.Service; import org.springframework.ui.freemarker.FreeMarkerTemplateUtils; import org.thingsboard.rule.engine.api.MailService; import org.thingsboard.rule.engine.api.TbEmail; +import org.thingsboard.rule.engine.mail.TbSendEmailNodeConfiguration; import org.thingsboard.server.common.data.AdminSettings; import org.thingsboard.server.common.data.ApiFeature; import org.thingsboard.server.common.data.ApiUsageRecordKey; @@ -49,6 +51,7 @@ import org.thingsboard.server.queue.usagestats.TbApiUsageClient; import org.thingsboard.server.service.apiusage.TbApiUsageStateService; import javax.annotation.PostConstruct; +import javax.annotation.PreDestroy; import javax.mail.MessagingException; import javax.mail.internet.MimeMessage; import java.io.ByteArrayInputStream; @@ -56,6 +59,8 @@ import java.util.HashMap; import java.util.Locale; import java.util.Map; import java.util.Properties; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -83,6 +88,11 @@ public class DefaultMailService implements MailService { @Autowired private MailExecutorService mailExecutorService; + @Value("${actors.rule.mail_timeout_thread_pool_size}") + private int timeoutExecutorPoolSize; + + private ExecutorService timeoutExecutorService; + private JavaMailSenderImpl mailSender; private String mailFrom; @@ -99,6 +109,14 @@ public class DefaultMailService implements MailService { @PostConstruct private void init() { updateMailConfiguration(); + timeoutExecutorService = Executors.newFixedThreadPool(timeoutExecutorPoolSize); + } + + @PreDestroy + private void destroy() { + if (timeoutExecutorService != null) { + timeoutExecutorService.shutdown(); + } } @Override @@ -262,15 +280,15 @@ public class DefaultMailService implements MailService { @Override public void send(TenantId tenantId, CustomerId customerId, TbEmail tbEmail) throws ThingsboardException { - sendMail(tenantId, customerId, tbEmail, this.mailSender); + sendMail(tenantId, customerId, tbEmail, this.mailSender, timeout); } @Override - public void send(TenantId tenantId, CustomerId customerId, TbEmail tbEmail, JavaMailSender javaMailSender) throws ThingsboardException { - sendMail(tenantId, customerId, tbEmail, javaMailSender); + public void send(TenantId tenantId, CustomerId customerId, TbEmail tbEmail, JavaMailSender javaMailSender, long timeout) throws ThingsboardException { + sendMail(tenantId, customerId, tbEmail, javaMailSender, timeout); } - private void sendMail(TenantId tenantId, CustomerId customerId, TbEmail tbEmail, JavaMailSender javaMailSender) throws ThingsboardException { + private void sendMail(TenantId tenantId, CustomerId customerId, TbEmail tbEmail, JavaMailSender javaMailSender, long timeout) throws ThingsboardException { if (apiUsageStateService.getApiUsageState(tenantId).isEmailSendEnabled()) { try { MimeMessage mailMsg = javaMailSender.createMimeMessage(); @@ -474,10 +492,11 @@ public class DefaultMailService implements MailService { } private void sendMailWithTimeout(JavaMailSender mailSender, MimeMessage msg, long timeout) { - var submittedMail = Futures.submit(() -> mailSender.send(msg), mailExecutorService); + var submittedMail = timeoutExecutorService.submit(() -> mailSender.send(msg)); try { submittedMail.get(timeout, TimeUnit.MILLISECONDS); - } catch (TimeoutException ignored) { + } catch (TimeoutException e) { + log.debug("Error during mail submission", e); throw new RuntimeException("Timeout!"); } catch (Exception e) { throw new RuntimeException(ExceptionUtils.getRootCause(e)); diff --git a/application/src/main/resources/thingsboard.yml b/application/src/main/resources/thingsboard.yml index 9b458734f7..aadcc6cebb 100644 --- a/application/src/main/resources/thingsboard.yml +++ b/application/src/main/resources/thingsboard.yml @@ -326,7 +326,9 @@ actors: # Specify thread pool size for javascript executor service js_thread_pool_size: "${ACTORS_RULE_JS_THREAD_POOL_SIZE:50}" # Specify thread pool size for mail sender executor service - mail_thread_pool_size: "${ACTORS_RULE_MAIL_THREAD_POOL_SIZE:50}" + mail_thread_pool_size: "${ACTORS_RULE_MAIL_THREAD_POOL_SIZE:40}" + # Specify thread pool size for mail sender executor service + mail_timeout_thread_pool_size: "${ACTORS_RULE_MAIL_TIMEOUT_THREAD_POOL_SIZE:10}" # Specify thread pool size for sms sender executor service sms_thread_pool_size: "${ACTORS_RULE_SMS_THREAD_POOL_SIZE:50}" # Whether to allow usage of system mail service for rules diff --git a/rule-engine/rule-engine-api/src/main/java/org/thingsboard/rule/engine/api/MailService.java b/rule-engine/rule-engine-api/src/main/java/org/thingsboard/rule/engine/api/MailService.java index c4b47d2e46..408ee8eaaf 100644 --- a/rule-engine/rule-engine-api/src/main/java/org/thingsboard/rule/engine/api/MailService.java +++ b/rule-engine/rule-engine-api/src/main/java/org/thingsboard/rule/engine/api/MailService.java @@ -47,8 +47,7 @@ public interface MailService { void sendTwoFaVerificationEmail(String email, String verificationCode, int expirationTimeSeconds) throws ThingsboardException; void send(TenantId tenantId, CustomerId customerId, TbEmail tbEmail) throws ThingsboardException; - - void send(TenantId tenantId, CustomerId customerId, TbEmail tbEmail, JavaMailSender javaMailSender) throws ThingsboardException; + void send(TenantId tenantId, CustomerId customerId, TbEmail tbEmail, JavaMailSender javaMailSender, long timeout) throws ThingsboardException; void sendApiFeatureStateEmail(ApiFeature apiFeature, ApiUsageStateValue stateValue, String email, ApiUsageStateMailMessage msg) throws ThingsboardException; diff --git a/rule-engine/rule-engine-components/src/main/java/org/thingsboard/rule/engine/mail/TbSendEmailNode.java b/rule-engine/rule-engine-components/src/main/java/org/thingsboard/rule/engine/mail/TbSendEmailNode.java index c7d1d6c900..bf65dc1fc3 100644 --- a/rule-engine/rule-engine-components/src/main/java/org/thingsboard/rule/engine/mail/TbSendEmailNode.java +++ b/rule-engine/rule-engine-components/src/main/java/org/thingsboard/rule/engine/mail/TbSendEmailNode.java @@ -88,7 +88,7 @@ public class TbSendEmailNode implements TbNode { if (this.config.isUseSystemSmtpSettings()) { ctx.getMailService(true).send(ctx.getTenantId(), msg.getCustomerId(), email); } else { - ctx.getMailService(false).send(ctx.getTenantId(), msg.getCustomerId(), email, this.mailSender); + ctx.getMailService(false).send(ctx.getTenantId(), msg.getCustomerId(), email, this.mailSender, config.getTimeout()); } } From a3ffd81348c7e328d6db2e0d646875fc30cfb446 Mon Sep 17 00:00:00 2001 From: ivankozka Date: Wed, 6 Jul 2022 13:46:02 +0300 Subject: [PATCH 4/4] remove redundant imports --- .../thingsboard/server/service/mail/DefaultMailService.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/application/src/main/java/org/thingsboard/server/service/mail/DefaultMailService.java b/application/src/main/java/org/thingsboard/server/service/mail/DefaultMailService.java index 8140c79189..fea3d25239 100644 --- a/application/src/main/java/org/thingsboard/server/service/mail/DefaultMailService.java +++ b/application/src/main/java/org/thingsboard/server/service/mail/DefaultMailService.java @@ -16,7 +16,6 @@ package org.thingsboard.server.service.mail; import com.fasterxml.jackson.databind.JsonNode; -import com.google.common.util.concurrent.Futures; import freemarker.template.Configuration; import freemarker.template.Template; import lombok.extern.slf4j.Slf4j; @@ -35,7 +34,6 @@ import org.springframework.stereotype.Service; import org.springframework.ui.freemarker.FreeMarkerTemplateUtils; import org.thingsboard.rule.engine.api.MailService; import org.thingsboard.rule.engine.api.TbEmail; -import org.thingsboard.rule.engine.mail.TbSendEmailNodeConfiguration; import org.thingsboard.server.common.data.AdminSettings; import org.thingsboard.server.common.data.ApiFeature; import org.thingsboard.server.common.data.ApiUsageRecordKey; @@ -52,7 +50,6 @@ import org.thingsboard.server.service.apiusage.TbApiUsageStateService; import javax.annotation.PostConstruct; import javax.annotation.PreDestroy; -import javax.mail.MessagingException; import javax.mail.internet.MimeMessage; import java.io.ByteArrayInputStream; import java.util.HashMap;