Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix concurrency issue in DialAndSendWithContext #386

Merged
merged 11 commits into from
Nov 22, 2024

Conversation

wneessen
Copy link
Owner

@wneessen wneessen commented Nov 22, 2024

This PR fixes concurrency issues when a user would use DialAndSend or DialAndSendWithContext in concurrent goroutines instead of using a single Client and then just using Send within the goroutines. With this PR the overall concurrency-safety is made more reliable. Goroutines are easy, but concurrency is hard, and since we added concurrency just with the latest release, some issues were to be expected.

In this PR the whole dial and send mechanic was refactored to use a dedicated, local SMTP client per DialAndSendWithContext call (as suggested by @firefart in #385). This fixes the concurrency issues that would arrise when the Client accesses the smtp.Client stored in the Client struct. To accomplish this, several public methods have been added that accept a smtp.Client pointer and use this instead of the Client.smtpClient shared on the Client struct. Additionally some private methods have been refactored to always accept a smtp.Client as well.

To avoid breaking any current code, the old methods have been refactored in a way that they make use of the new smtp.Client-accepting methods, but they will still hand over the Client.smtpClient to these functions, but with an additional mutex for sending. This way we should provide full compatibility with any code that was written before this PR.

This addresses #385.

Updated tests to correctly assert the absence of an SMTP client on failure. Added concurrent sending tests for DialAndSendWithContext to improve test coverage and reliability. Also, refined the `AutoDiscover` and other client methods to ensure proper parameter use.
Mutex locks are added to ensure thread safety when setting DSN mail return and recipient notify options. This prevents data races in concurrent environments, improving the client's robustness.
Refactor `DialWithContext` to delegate client creation to `SMTPClientFromDialWithContext`. Add new methods `SendWithSMTPClient`, `CloseWithSMTPClient`, and `ResetWithSMTPClient` to handle specific actions on `smtp.Client`. Simplify `auth`, `sendSingleMsg`, and `tls` methods by passing client as parameters.
@wneessen wneessen linked an issue Nov 22, 2024 that may be closed by this pull request
@wneessen wneessen marked this pull request as ready for review November 22, 2024 14:05
Separated debug logging and logger setting methods to include SMTP client parameter for better encapsulation. Removed commented-out code for cleaner and more manageable codebase.
Added a mutex lock and unlock around the Send function to prevent concurrent access issues and ensure thread safety. This change helps avoid race conditions when multiple goroutines attempt to send messages simultaneously.
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 99.14530% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.86%. Comparing base (bc5d980) to head (6ebc60d).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
client.go 99.07% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #386      +/-   ##
==========================================
+ Coverage   96.64%   96.86%   +0.22%     
==========================================
  Files          28       28              
  Lines        3100     3128      +28     
==========================================
+ Hits         2996     3030      +34     
+ Misses         72       68       -4     
+ Partials       32       30       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

The new Send function in client.go adds thread safety by using a mutex. This change also removes duplicate Send functions from client_119.go and client_120.go, consolidating the logic in one place for easier maintenance.
Updated several Client methods to accept a smtp.Client pointer, allowing for more flexible and explicit SMTP client management. Added detailed parameter descriptions and extended error handling documentation.
Renamed `SMTPClientFromDialWithContext` to `DialToSMTPClientWithContext` for clarity and consistency. Updated method parameters and documentation to standardize context usage across the codebase.
Introduce a new test, `TestClient_DialToSMTPClientWithContext`, to verify client behavior when establishing and closing an SMTP connection with context handling. Also added a sub-test to simulate and confirm failure scenarios during SMTP connection establishment.
This change removes an unnecessary blank line at the end of the TestClient_sendSingleMsg function in the client_test.go file. Keeping the code clean and properly formatted improves readability and maintainability.
The removed check for a nil SMTP client was redundant because the previous error handling already covers this case. Streamlining this part of the code improves readability and reduces unnecessary checks.
@wneessen wneessen merged commit ead4067 into main Nov 22, 2024
29 checks passed
@wneessen wneessen deleted the bug/385_concurrency-issue-in-dialandsendwithcontext branch November 22, 2024 15:36
@firefart
Copy link

Awesome thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Concurrency Issue in DialAndSendWithContext
2 participants