Fixed async error - #136
Conversation
📝 WalkthroughWalkthroughSpring letter delivery now uses an asynchronously configured ChangesAsynchronous letter mail delivery
Repository ignore configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LetterService
participant MailDispatcher
participant JavaMailSender
participant MeterRegistry
LetterService->>MailDispatcher: sendAsync(to, subject, html)
MailDispatcher->>JavaMailSender: Create and send HTML message
JavaMailSender-->>MailDispatcher: Send result
MailDispatcher->>MeterRegistry: Increment success or failure counter
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
services/spring-letter/src/main/java/tum/devoops/letterservice/config/AsyncConfig.java (1)
16-22: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrevent dropped emails on application shutdown.
Since this executor handles a queue of background mail tasks, any tasks left in the queue when the application shuts down will be immediately discarded by default.
Consider setting
setWaitForTasksToCompleteOnShutdown(true)to ensure that queued emails are sent before the application exits.♻️ Proposed refactor
ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor(); executor.setCorePoolSize(2); executor.setMaxPoolSize(10); executor.setQueueCapacity(500); executor.setThreadNamePrefix("mail-dispatch-"); + executor.setWaitForTasksToCompleteOnShutdown(true); + executor.setAwaitTerminationSeconds(60); executor.initialize(); return executor;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/spring-letter/src/main/java/tum/devoops/letterservice/config/AsyncConfig.java` around lines 16 - 22, Update the ThreadPoolTaskExecutor configuration in AsyncConfig to call setWaitForTasksToCompleteOnShutdown(true) before initialize(), ensuring queued mail tasks finish during application shutdown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@services/spring-letter/src/main/java/tum/devoops/letterservice/service/MailDispatcher.java`:
- Around line 42-45: Update the exception handling in MailDispatcher’s
mail-sending flow to catch Spring MailException, or a broader Exception, so
runtime delivery failures from mailSender.send(message) increment the failure
counter and are logged through the existing LOG.error call. Preserve the current
failure metric and logging behavior for MessagingException as well.
---
Nitpick comments:
In
`@services/spring-letter/src/main/java/tum/devoops/letterservice/config/AsyncConfig.java`:
- Around line 16-22: Update the ThreadPoolTaskExecutor configuration in
AsyncConfig to call setWaitForTasksToCompleteOnShutdown(true) before
initialize(), ensuring queued mail tasks finish during application shutdown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 30941703-8e55-42d2-81f7-463c5f446ecb
📒 Files selected for processing (6)
.gitignoreservices/spring-letter/src/main/java/tum/devoops/letterservice/LetterServiceApplication.javaservices/spring-letter/src/main/java/tum/devoops/letterservice/config/AsyncConfig.javaservices/spring-letter/src/main/java/tum/devoops/letterservice/service/LetterService.javaservices/spring-letter/src/main/java/tum/devoops/letterservice/service/MailDispatcher.javaservices/spring-letter/src/test/java/tum/devoops/letterservice/service/LetterServiceTest.java
| } catch (MessagingException e) { | ||
| meterRegistry.counter("letters_sent_total", "status", "failure").increment(); | ||
| LOG.error("Failed to send mail to {}", to, e); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Catch Spring's MailException to properly track sending failures.
mailSender.send(message) throws Spring's MailException (an unchecked RuntimeException), not the checked MessagingException (which is only thrown by MimeMessageHelper setup methods). By only catching MessagingException, actual delivery failures (like connection refused or SMTP timeouts) will bypass this catch block. This will cause the failure metric to be missed, and the exception will be swallowed by Spring's async executor.
Catch Exception (or add a catch block for MailException) to ensure all delivery failures are correctly logged and metered.
🐛 Proposed fix
- } catch (MessagingException e) {
+ } catch (Exception e) {
meterRegistry.counter("letters_sent_total", "status", "failure").increment();
LOG.error("Failed to send mail to {}", to, e);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (MessagingException e) { | |
| meterRegistry.counter("letters_sent_total", "status", "failure").increment(); | |
| LOG.error("Failed to send mail to {}", to, e); | |
| } | |
| } catch (Exception e) { | |
| meterRegistry.counter("letters_sent_total", "status", "failure").increment(); | |
| LOG.error("Failed to send mail to {}", to, e); | |
| } |
🧰 Tools
🪛 PMD (7.26.0)
[Low] 44-44: InvalidLogMessageFormat (Error Prone): Too many arguments, expected 1 argument but found 2
(InvalidLogMessageFormat (Error Prone))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@services/spring-letter/src/main/java/tum/devoops/letterservice/service/MailDispatcher.java`
around lines 42 - 45, Update the exception handling in MailDispatcher’s
mail-sending flow to catch Spring MailException, or a broader Exception, so
runtime delivery failures from mailSender.send(message) increment the failure
counter and are logged through the existing LOG.error call. Preserve the current
failure metric and logging behavior for MessagingException as well.
2 participants
Changed mail sending to async to fix that false errors are shown when emails are sent.
Summary by CodeRabbit
New Features
Bug Fixes
Chores