employee-dev-14-BE-setup-thread-for-email-service #209
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @yennanliu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a robust asynchronous email service to the backend. By leveraging dedicated thread pools, email sending operations are now non-blocking, improving application responsiveness and scalability. The changes include new configuration for async executors, a high-level email service facade, enhancements to the low-level mail sender, and integration into the vacation request flow, along with documentation and tests.
Highlights
- Asynchronous Email Processing: I've implemented asynchronous email sending using Spring's
@Asyncannotation andCompletableFutureto prevent email operations from blocking the main application threads. - Dedicated Thread Pools: I've configured dedicated thread pools in
AsyncConfig.javafor email tasks (emailTaskExecutor) and future notification tasks (notificationTaskExecutor), separate from the default async executor. This provides better resource isolation and control. - Email Service Facade: A new
EmailService.javaclass acts as a high-level facade for various email types (vacation notifications, admin notifications, status updates, welcome emails), abstracting the low-level mail sending details. - Enhanced Mail Service: The
MailService.javahas been updated to use the dedicatedemailTaskExecutor, include more detailed logging with timestamps and thread names, support HTML content, and include placeholders for retry/failure handling. - Vacation Service Integration: The
VacationService.javanow uses the newEmailServicefacade to send admin notifications asynchronously when a new vacation request is added. - Documentation and Testing: I've added a comprehensive
EMAIL_SERVICE_DOCUMENTATION.mdfile explaining the new email service architecture and usage, and included unit tests inEmailServiceTest.javafor theEmailServicemethods.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configureGemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated asynchronous email service, enhancing the system by offloading email operations to separate thread pools. Key changes include the new EmailService facade, updates to MailService and AsyncConfig, and integration into VacationService. Documentation and unit tests for the email service are also added.
My review highlights several areas for improvement:
- The thread pool's
CallerRunsPolicycould negatively impact application performance under load. - There are concerns regarding the correctness of asynchronous operations, particularly how
CompletableFutures fromEmailServicereflect the actual email sending status, and how exceptions are propagated fromMailService. - A critical issue was found in the
sendMailWithRetrylogic, which currently does not function as intended. - The absence of user notifications for vacation submissions is a significant point for user experience.
Suggestions are provided to address these points, aiming to improve the robustness, correctness, and reliability of the new email service.
| @Async("emailTaskExecutor") | ||
| public void sendMailWithRetry(NotificationEmail notificationEmail, int maxRetries) { | ||
| String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss")); | ||
| String threadName = Thread.currentThread().getName(); | ||
|
|
||
| log.info("[{}] Starting email sending with retry in thread: {} for recipient: {} (max retries: {})", | ||
| timestamp, threadName, notificationEmail.getRecipient(), maxRetries); | ||
|
|
||
| for (int attempt = 1; attempt <= maxRetries; attempt++) { | ||
| try { | ||
| sendMail(notificationEmail); | ||
| log.info("[{}] Email sent successfully on attempt {} for: {}", | ||
| timestamp, attempt, notificationEmail.getRecipient()); | ||
| return; // Success, exit retry loop | ||
|
|
||
| } catch (Exception e) { | ||
| log.warn("[{}] Email sending attempt {} failed for: {} - Error: {}", | ||
| timestamp, attempt, notificationEmail.getRecipient(), e.getMessage()); | ||
|
|
||
| if (attempt == maxRetries) { | ||
| log.error("[{}] All {} email sending attempts failed for: {}", | ||
| timestamp, maxRetries, notificationEmail.getRecipient()); | ||
| handleEmailFailure(notificationEmail, e, timestamp); | ||
| } else { | ||
| // Wait before retry (exponential backoff) | ||
| try { | ||
| Thread.sleep(1000 * attempt); // Wait 1s, 2s, 3s, etc. | ||
| } catch (InterruptedException ie) { | ||
| Thread.currentThread().interrupt(); | ||
| log.error("Email retry interrupted for: {}", notificationEmail.getRecipient()); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The sendMailWithRetry method's current implementation will not achieve the intended retry behavior. It calls sendMail(notificationEmail) which is an @Async void method. This means sendMailWithRetry schedules an asynchronous task and proceeds without waiting for its completion or failure. Consequently, the try-catch block within the retry loop will only catch exceptions if the submission of the sendMail task fails, not if the asynchronous email sending operation itself fails.
To correctly implement retries:
- A synchronous, private method should encapsulate the actual email sending logic.
sendMailWithRetryshould then call this synchronous method within its retry loop. AnyMailExceptionthrown would be caught by thetry-catchinsendMailWithRetry, allowing the retry logic to function correctly.
| executor.setAllowCoreThreadTimeOut(true); | ||
|
|
||
| // Rejection policy when pool and queue are full | ||
| executor.setRejectedExecutionHandler(new ThreadPoolExecutor.CallerRunsPolicy()); |
There was a problem hiding this comment.
The CallerRunsPolicy rejection handler for the emailTaskExecutor can cause the thread that submitted the task to execute the task itself if the thread pool and its queue are full. This can lead to the calling thread being blocked, potentially degrading overall application responsiveness.
Consider alternative rejection policies like ThreadPoolExecutor.AbortPolicy or a custom policy.
| @Async("emailTaskExecutor") | ||
| public void sendMail(NotificationEmail notificationEmail) { | ||
|
|
||
| String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss")); | ||
| String threadName = Thread.currentThread().getName(); | ||
|
|
||
| log.info("[{}] Starting email sending process in thread: {} for recipient: {}", | ||
| timestamp, threadName, notificationEmail.getRecipient()); | ||
|
|
||
| MimeMessagePreparator messagePreparator = mimeMessage -> { | ||
| try { | ||
| MimeMessageHelper messageHelper = new MimeMessageHelper(mimeMessage, true, "UTF-8"); | ||
| messageHelper.setFrom(adminEmail); | ||
| messageHelper.setTo(notificationEmail.getRecipient()); | ||
| messageHelper.setSubject(notificationEmail.getSubject()); | ||
|
|
||
| // Use HTML content if available, otherwise use plain text | ||
| String emailBody = mailContentBuilder.build(notificationEmail.getBody()); | ||
| messageHelper.setText(emailBody, true); // true indicates HTML content | ||
|
|
||
| log.debug("[{}] Email message prepared for: {}", timestamp, notificationEmail.getRecipient()); | ||
|
|
||
| } catch (Exception e) { | ||
| log.error("[{}] Error preparing email message: {}", timestamp, e.getMessage()); | ||
| throw new RuntimeException("Failed to prepare email message", e); | ||
| } | ||
| }; | ||
|
|
||
| try { | ||
| // Send the email | ||
| mailSender.send(messagePreparator); | ||
| log.info("email sent!!"); | ||
|
|
||
| log.info("[{}] ✅ Email sent successfully in thread: {} to: {} with subject: '{}'", | ||
| timestamp, threadName, notificationEmail.getRecipient(), notificationEmail.getSubject()); | ||
|
|
||
| } catch (MailException e) { | ||
| log.error("Exception occurred when sending mail", e); | ||
| e.printStackTrace(); | ||
| throw new CustomException( | ||
| "Exception occurred when sending mail to " + notificationEmail.getRecipient() + e); | ||
| log.error("[{}] ❌ Mail sending failed in thread: {} for recipient: {} - Error: {}", | ||
| timestamp, threadName, notificationEmail.getRecipient(), e.getMessage()); | ||
|
|
||
| // Don't throw exception here to prevent breaking the async flow | ||
| // Instead, log the error and potentially implement retry logic or dead letter queue | ||
| handleEmailFailure(notificationEmail, e, timestamp); | ||
| } |
There was a problem hiding this comment.
The sendMail method's current design combined with its exception handling has implications for callers like EmailService:
-
EmailServicemethods return aCompletableFuture. However, becauseMailService.sendMailisvoidand async, the future fromEmailServicecompletes as soon asMailService.sendMailis scheduled, not when the email operation actually finishes or fails. -
MailExceptions are caught and logged, but not propagated. This means that if an email fails to send,EmailServiceand its callers will not be aware of the failure through theCompletableFuture.
To address these issues, MailService.sendMail should return CompletableFuture<Void>. This future should complete normally on successful email dispatch and exceptionally on failure.
@Async("emailTaskExecutor")
public CompletableFuture<Void> sendMail(NotificationEmail notificationEmail) {
String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss"));
String threadName = Thread.currentThread().getName();
log.info("[{}] Starting email sending process in thread: {} for recipient: {}",
timestamp, threadName, notificationEmail.getRecipient());
try {
MimeMessagePreparator messagePreparator = mimeMessage -> {
MimeMessageHelper messageHelper = new MimeMessageHelper(mimeMessage, true, "UTF-8");
messageHelper.setFrom(adminEmail);
messageHelper.setTo(notificationEmail.getRecipient());
messageHelper.setSubject(notificationEmail.getSubject());
String emailBody = mailContentBuilder.build(notificationEmail.getBody());
messageHelper.setText(emailBody, true); // true indicates HTML content
log.debug("[{}] Email message prepared for: {}", timestamp, notificationEmail.getRecipient());
};
mailSender.send(messagePreparator);
log.info("[{}] ✅ Email sent successfully in thread: {} to: {} with subject: '{}'",
timestamp, threadName, notificationEmail.getRecipient(), notificationEmail.getSubject());
return CompletableFuture.completedFuture(null);
} catch (MailException e) {
log.error("[{}] ❌ Mail sending failed in thread: {} for recipient: {} - Error: {}",
timestamp, threadName, notificationEmail.getRecipient(), e.getMessage(), e);
handleEmailFailure(notificationEmail, e, timestamp);
return CompletableFuture.failedFuture(e);
} catch (Exception e) { // Catch other potential runtime exceptions
log.error("[{}] ❌ Unexpected error during email sending process for recipient: {} - Error: {}",
timestamp, threadName, notificationEmail.getRecipient(), e.getMessage(), e);
handleEmailFailure(notificationEmail, e, timestamp);
return CompletableFuture.failedFuture(e);
}
}| vacation.getEndDate().toString() | ||
| ); | ||
|
|
||
| // TODO: Send notification to user (need to get user email from userId) |
There was a problem hiding this comment.
The TODO comment on this line points to a significant gap in functionality: the user who submits a vacation request is not currently notified by email. This should be considered a high-priority item to implement.
There was a problem hiding this comment.
/gemini offer the code to implement the description in comment
There was a problem hiding this comment.
Certainly. Implementing the user notification requires fetching the user's email address based on the userId. This would typically involve calling a method from a UserService or similar component.
Assuming you have a way to retrieve the user's email (e.g., userService.getUserEmail(vacation.getUserId())), here is a possible implementation snippet you could add after the admin notification logic:
// Fetch user email based on vacation.getUserId()
String userEmail = userService.getUserEmail(vacation.getUserId()); // Assuming a userService exists and has this method
if (userEmail != null) {
// Send notification to user
emailService.sendUserNotificationAsync(
userEmail,
vacation.getUserId(),
vacation.getType(),
vacation.getStartDate().toString(),
vacation.getEndDate().toString()
// You might want to include more details in the email body, similar to the admin notification
);
} else {
log.warn("Could not find email for user ID: " + vacation.getUserId() + ". User notification skipped.");
}You would also need to add the sendUserNotificationAsync method to your EmailService to handle sending the email to the user asynchronously, similar to how sendAdminNotificationAsync is implemented.
| ```properties | ||
| # Mail Properties | ||
| spring.mail.host=smtp.mailtrap.io | ||
| spring.mail.port=25 |
There was a problem hiding this comment.
Consider updating the documentation to recommend and exemplify configuration for SMTPS (e.g., port 465) or SMTP with STARTTLS (e.g., port 587) for enhanced security. This would involve properties like spring.mail.properties.mail.smtp.auth=true, spring.mail.properties.mail.smtp.starttls.enable=true, or spring.mail.properties.mail.smtp.ssl.enable=true, depending on the mail server's requirements.
| emailService.sendAdminNotificationAsync( | ||
| adminEmail, | ||
| vacation.getUserId(), | ||
| vacation.getType(), | ||
| vacation.getStartDate().toString(), | ||
| vacation.getEndDate().toString() | ||
| ); |
There was a problem hiding this comment.
The CompletableFuture<Void> returned by emailService.sendAdminNotificationAsync is currently not handled. Consider adding handlers like .thenRun() for success and .exceptionally() for failure to improve the system's observability and robustness.
| emailService.sendAdminNotificationAsync( | |
| adminEmail, | |
| vacation.getUserId(), | |
| vacation.getType(), | |
| vacation.getStartDate().toString(), | |
| vacation.getEndDate().toString() | |
| ); | |
| emailService.sendAdminNotificationAsync( | |
| adminEmail, | |
| vacation.getUserId(), | |
| vacation.getType(), | |
| vacation.getStartDate().toString(), | |
| vacation.getEndDate().toString() | |
| ).thenRun(() -> | |
| log.info("Admin notification email for vacation by user {} successfully queued.", vacation.getUserId()) | |
| ).exceptionally(ex -> { | |
| log.error("Failed to send admin notification email for vacation by user {}: {}", vacation.getUserId(), ex.getMessage(), ex); | |
| return null; | |
| }); |
| @Test | ||
| void testEmailBodyBuilding() { | ||
| // This test verifies that email bodies are constructed properly | ||
| // by checking that the service methods complete without throwing exceptions | ||
|
|
||
| String userEmail = "[email protected]"; | ||
| Integer userId = 100; | ||
| String vacationType = "Test Leave"; | ||
| String startDate = "2024-01-01"; | ||
| String endDate = "2024-01-05"; | ||
|
|
||
| doNothing().when(mailService).sendMail(any(NotificationEmail.class)); | ||
|
|
||
| // Test all email types | ||
| assertDoesNotThrow(() -> { | ||
| emailService.sendVacationNotificationAsync(userEmail, userId, vacationType, startDate, endDate); | ||
| emailService.sendAdminNotificationAsync("[email protected]", userId, vacationType, startDate, endDate); | ||
| emailService.sendVacationStatusUpdateAsync(userEmail, userId, vacationType, "APPROVED", startDate, endDate); | ||
| emailService.sendWelcomeEmailAsync(userEmail, "Test", "User"); | ||
| }); | ||
|
|
||
| // Verify all methods were called | ||
| verify(mailService, times(4)).sendMail(any(NotificationEmail.class)); | ||
| } |
There was a problem hiding this comment.
The testEmailBodyBuilding method verifies that mailService.sendMail is called, but it doesn't assert the content of the email being sent. To make this test more robust, you can use Mockito's ArgumentCaptor<NotificationEmail> to capture the NotificationEmail object passed to mailService.sendMail and assert that the subject and body of the email are generated as expected.
void testEmailBodyBuilding() {
String userEmail = "[email protected]";
Integer userId = 100;
String vacationType = "Test Leave";
String startDate = "2024-01-01";
String endDate = "2024-01-05";
String adminEmail = "[email protected]";
String status = "APPROVED";
String firstName = "Test";
String lastName = "User";
ArgumentCaptor<NotificationEmail> emailCaptor = ArgumentCaptor.forClass(NotificationEmail.class);
doNothing().when(mailService).sendMail(emailCaptor.capture());
assertDoesNotThrow(() -> {
emailService.sendVacationNotificationAsync(userEmail, userId, vacationType, startDate, endDate).join();
NotificationEmail vacationNotification = emailCaptor.getValue();
assertTrue(vacationNotification.getSubject().contains("Vacation Request Submitted"));
assertTrue(vacationNotification.getBody().contains("Dear Employee (ID: " + userId + ")"));
assertTrue(vacationNotification.getBody().contains("Type: " + vacationType));
emailService.sendAdminNotificationAsync(adminEmail, userId, vacationType, startDate, endDate).join();
NotificationEmail adminNotification = emailCaptor.getValue();
assertTrue(adminNotification.getSubject().contains("New Vacation Request"));
assertTrue(adminNotification.getBody().contains("Employee ID: " + userId));
emailService.sendVacationStatusUpdateAsync(userEmail, userId, vacationType, status, startDate, endDate).join();
NotificationEmail statusUpdate = emailCaptor.getValue();
assertTrue(statusUpdate.getSubject().contains("Vacation Request " + status));
assertTrue(statusUpdate.getBody().contains("Your vacation request has been " + status.toLowerCase()));
emailService.sendWelcomeEmailAsync(userEmail, firstName, lastName).join();
NotificationEmail welcomeEmail = emailCaptor.getValue();
assertTrue(welcomeEmail.getSubject().contains("Welcome to Employee System"));
assertTrue(welcomeEmail.getBody().contains("Dear " + firstName + " " + lastName));
});
verify(mailService, times(4)).sendMail(any(NotificationEmail.class));
}
No description provided.