Repository navigation
notify/email: honour the context when dialing with implicit TLS - #5690
Open
vignesh2027 wants to merge 1 commit into
Open
vignesh2027 wants to merge 1 commit into
vignesh2027 wants to merge 1 commit into
Conversation
The plain SMTP path dials with DialContext, but the implicit TLS path used tls.Dial, which has no timeout and ignores the context. A smarthost that accepts the TCP connection but never completes the TLS handshake blocked the notification with no limit. Use tls.Dialer.DialContext so both the dial and the handshake stop when the notification context is done. Signed-off-by: vigneshwar <applemacbook6sep2004@gmail.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Context-aware implicit-TLS dialing notify/email/email.go, notify/email/email_test.go |
Email.Notify uses tls.Dialer.DialContext with the notification context for implicit-TLS connections. The test checks cancellation during a stalled handshake and requires the call to return within five seconds. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Merge Risk: 
4afd4
-
Implicit-TLS notification dialing now honors cancellation, and no actionable merge risk is identified.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies the email notifier area and explains that implicit TLS now honors the context during dialing. Description check The description is complete and relevant. It explains the bug, the implementation, the regression test, compatibility impact, checklist status, and release note. Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
TheMeier
self-requested a review
October 26, 2026 05:51
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist
Please check all the applicable boxes.
benchstatto compare benchmarksThe email notifier connects with
net.Dialer.DialContextwhen it uses plain SMTP or STARTTLS, but withtls.Dialwhen implicit TLS is used (port 465 orforce_implicit_tls: true).tls.Dialhas no timeout and does not look at the context.If the smarthost accepts the TCP connection but never finishes the TLS handshake,
Notifystays blocked even after the notification context is done. The existingTestEmailNotifyWithDoneContextcovers this for the plain path only.This change uses
tls.Dialer.DialContext, so the dial and the handshake both stop when the context ends. The new test starts a listener that accepts connections and never writes, then callsNotifywith a 100ms context timeout. On main it is still blocked after 5 seconds. With the fix it returnsestablish TLS connection to server: ...right after the timeout.Which user-facing changes does this PR introduce?