HTM-1820: Fix blocking when sending a password-reset email #1534
Conversation
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1534 +/- ##
========================================
- Coverage 77% 4% -73%
Complexity 105 105
========================================
Files 151 153 +2
Lines 7576 7582 +6
Branches 690 691 +1
========================================
- Hits 5815 267 -5548
- Misses 1361 7296 +5935
+ Partials 400 19 -381
... and 138 files with indirect coverage changes 🚀 New features to boost your workflow:
|
matthijsln
changed the title
matthijsln
force-pushed
the
password-reset-dont-block-send
branch
from
c990402 to
598020b
Compare
…shut down ExecutorService after creating and submitting a task)
matthijsln
force-pushed
the
password-reset-dont-block-send
branch
from
598020b to
a641c07
Compare
mprins
left a comment
mprins
left a comment
There was a problem hiding this comment.
tests are failing:
Parameter 0 of constructor in org.tailormap.api.service.PasswordResetEmailService required a bean of type 'org.springframework.mail.javamail.JavaMailSender' that could not be found.
There was a problem hiding this comment.
Pull request overview
This PR fixes a blocking issue in the password reset email functionality by replacing manual ExecutorService management with Spring's built-in async support. The original implementation immediately shut down the ExecutorService after submitting tasks, which prevented email sending from completing.
Key changes:
- Introduced Spring's @EnableAsync configuration with a dedicated ThreadPoolTaskExecutor for password reset emails
- Extracted email sending logic into a new PasswordResetEmailService with @async method
- Removed manual ExecutorService creation and management from PasswordResetController
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/java/org/tailormap/api/configuration/AsyncConfig.java | New configuration class that enables async support and defines a dedicated ThreadPoolTaskExecutor bean for password reset operations |
| src/main/java/org/tailormap/api/service/PasswordResetEmailService.java | New service class that encapsulates password reset email sending logic with @async annotation for proper async execution |
| src/main/java/org/tailormap/api/controller/PasswordResetController.java | Refactored to delegate email sending to the new PasswordResetEmailService, removing manual ExecutorService management and related dependencies |
| public void sendPasswordResetEmailAsync( | ||
| String email, String absoluteLinkPrefix, Locale locale, int tokenExpiryMinutes) { | ||
| try { | ||
| User user = userRepository.findByEmail(email).orElse(null); | ||
| if (user == null || !user.isEnabledAndValidUntil()) { | ||
| return; | ||
| } | ||
|
|
||
| TemporaryToken token = | ||
| new TemporaryToken(TemporaryToken.TokenType.PASSWORD_RESET, user.getUsername(), tokenExpiryMinutes); | ||
| token = temporaryTokenRepository.save(token); | ||
|
|
||
| String absoluteLink = | ||
| absoluteLinkPrefix + "/user/password-reset/" + token.getCombinedTokenAndExpirationAsBase64(); | ||
|
|
||
| SimpleMailMessage message = new SimpleMailMessage(); | ||
| message.setFrom(mailFrom); | ||
| message.setTo(user.getEmail()); | ||
| message.setSubject(messageSource.getMessage("reset-password-request.email-subject", null, locale)); | ||
| message.setText( | ||
| messageSource.getMessage("reset-password-request.email-body", new Object[] {absoluteLink}, locale)); | ||
|
|
||
| logger.info("Sending password reset email for user: {}", user.getUsername()); | ||
| emailSender.send(message); // blocking, but run in async thread | ||
| } catch (Exception e) { | ||
| logger.error("Failed to send password reset email", e); | ||
| } |
There was a problem hiding this comment.
The async method performs database operations (userRepository.findByEmail and temporaryTokenRepository.save) without @transactional annotation. When running in a separate thread, these operations may not have proper transaction boundaries, which could lead to connection leaks or unexpected behavior. Consider adding @transactional annotation to this method to ensure proper transaction management in the async context.
| executor.setCorePoolSize(1); | ||
| executor.setMaxPoolSize(1); | ||
| executor.setQueueCapacity(10); | ||
| executor.setThreadNamePrefix("pwd-reset-"); |
There was a problem hiding this comment.
The AsyncConfig should configure setWaitForTasksToCompleteOnShutdown(true) and setAwaitTerminationSeconds to ensure graceful shutdown. Without this, pending password reset emails might not be sent when the application shuts down, potentially leaving users without the ability to reset their passwords. This is especially important given the queue capacity of 10.
| executor.setThreadNamePrefix("pwd-reset-"); | |
| executor.setThreadNamePrefix("pwd-reset-"); | |
| executor.setWaitForTasksToCompleteOnShutdown(true); | |
| executor.setAwaitTerminationSeconds(30); |
Do not immediately shut down the ExecutorService after creating and submitting a task.