-
Notifications
You must be signed in to change notification settings - Fork 265
fix: Prevent data race in email sending #494
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
Open
andrew-pavlov-ua
wants to merge
5
commits into
sendgrid:main
Choose a base branch
from
apvsyp:main-copy
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
4094410
Added a test to check thread safety, it failed with race 'detected du…
andrew-pavlov-ua 7f43e6f
Fixed data race when sending emails by changing Client struct
andrew-pavlov-ua 68ba9a0
Added -race in go.coverage.sh
andrew-pavlov-ua 2654985
Update twilio_email_test.go
tiwarishubham635 1703b14
Merge branch 'main' into main-copy
tiwarishubham635 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import ( | |
| "os" | ||
| "strconv" | ||
| "strings" | ||
| "sync" | ||
| "testing" | ||
| "time" | ||
|
|
||
|
|
@@ -1640,10 +1641,52 @@ func Test_test_mail_batch__batch_id__get(t *testing.T) { | |
| assert.Equal(t, 200, response.StatusCode, "Wrong status code returned") | ||
| } | ||
|
|
||
| func Test_test_client_send_is_thread_safe(t *testing.T) { | ||
| apiKey := "SENDGIRD_APIKEY" | ||
| const numRequests = 5 | ||
| var wg sync.WaitGroup | ||
| client := NewSendClient(apiKey) | ||
|
|
||
| // Launch multiple goroutines to send concurrent requests | ||
| for i := 0; i < numRequests; i++ { | ||
| wg.Add(1) | ||
| go func(i int) { | ||
| defer wg.Done() | ||
| from := &mail.Email{ | ||
| Name: "Name", | ||
| Address: "[email protected]", | ||
| } | ||
|
|
||
| to := &mail.Email{ | ||
| Name: "Recipient", | ||
| Address: "[email protected]", | ||
| } | ||
|
|
||
| email := &mail.SGMailV3{ | ||
| From: from, | ||
| Personalizations: []*mail.Personalization{ | ||
| { | ||
| To: []*mail.Email{to}, | ||
| Subject: "Subject", | ||
| }, | ||
| }, | ||
| Content: []*mail.Content{ | ||
| { | ||
| Type: "text/plain", | ||
| Value: "Value", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| client.Send(email) | ||
| }(i) | ||
| } | ||
| wg.Wait() | ||
| } | ||
|
|
||
| func Test_test_send_client_with_mail_body_compression_enabled(t *testing.T) { | ||
| apiKey := "SENDGRID_API_KEY" | ||
| client := NewSendClient(apiKey) | ||
| client.Headers["Content-Encoding"] = "gzip" | ||
|
|
||
| emailBytes := []byte(` { | ||
| "asm": { | ||
|
|
@@ -1780,8 +1823,10 @@ func Test_test_send_client_with_mail_body_compression_enabled(t *testing.T) { | |
| email := &mail.SGMailV3{} | ||
| err := json.Unmarshal(emailBytes, email) | ||
| assert.Nil(t, err, fmt.Sprintf("Unmarshal error: %v", err)) | ||
| client.Request.Headers["X-Mock"] = "202" | ||
| response, err := client.Send(email) | ||
|
|
||
| headers := map[string]string{"Content-Encoding": "gzip", "X-Mock": "202"} | ||
|
|
||
| response, err := client.SendWithHeaders(email, headers) | ||
| if err != nil { | ||
| t.Log(err) | ||
| } | ||
|
|
@@ -1929,8 +1974,7 @@ func Test_test_send_client(t *testing.T) { | |
| email := &mail.SGMailV3{} | ||
| err := json.Unmarshal(emailBytes, email) | ||
| assert.Nil(t, err, fmt.Sprintf("Unmarshal error: %v", err)) | ||
| client.Request.Headers["X-Mock"] = "202" | ||
| response, err := client.Send(email) | ||
| response, err := client.SendWithHeaders(email, map[string]string{"X-Mock": "202"}) | ||
| if err != nil { | ||
| t.Log(err) | ||
| } | ||
|
|
||
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change breaks backward compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to keep backward compatibility, but I broke it to remove the data race, I removed the public "Request" field from the client structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the maintainers are not concerned with keeping backward compatibility and do not adhere to semver.org. For example, the function
SetHostwas removed, but the major version was not updated: https://github.com/sendgrid/sendgrid-go/blob/main/CHANGELOG.md#2023-12-01-version-3140.Therefore, I think it should be acceptable for them if you change the parameters.
However, I suggest mentioning this change in the first few sentences of the PR's description. This way, anyone who comes across this PR will know how to fix their code without having to dive deeply into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we'd prefer if this change can be made backward compatible.