fix: Prevent data race in email sending

[andrew-pavlov-ua]

Fixes

When trying to send several emails at the same time, the emails are not sent because the client uses a shared built-in field rest.Request, which is overwritten by different goroutines.

// Client is the Twilio SendGrid Go client
type Client struct {
	rest.Request
}

// SendWithContext sends an email through Twilio SendGrid with context.Context
func (cl *Client) SendWithContext(ctx context.Context, email *mail.SGMailV3) (*rest.Response, error) {
	cl.Request.Body = mail.GetRequestBody(email)

	// ...
}

Here is a test I wrote to show this:

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()
}

Started the test using the following command:

go test ./... -v -run=Test_test_client_send_is_thread_safe -race

Here is the console output after running the test before applying my fixes:

testing.go:1399: race detected during execution of test
--- FAIL: Test_test_client_send_is_thread_safe (0.83s)

After my fixes, the test passes, emails are successfully sent, and the client is now thread-safe.

I replaced the embedded Request in Client with two private fields: apiKey and emailOptions. When creating a client, either apiKey or Username and Password are required. Backward compatibility is broken, but this is a necessary step to eliminate Data Race and make internal fields private.

New Implementation:

// Client is the Twilio SendGrid Go client
type Client struct {
	apiKey       string
	emailOptions TwilioEmailOptions
}

I also created the SendWithHeaders method to support additional headers (used in testing). Additionally, I refactored the client creation methods (NewSendClient and NewTwilioEmailSendClient) accordingly.

NewApiKeySendClient:

// NewSendClient constructs a new Twilio SendGrid client given an API key
func NewSendClient(key string) *Client {
	return &Client{apiKey: key}
}

NewEmailSendClient:

// NewTwilioEmailSendClient constructs a new Twilio Email client given a username and password
func NewTwilioEmailSendClient(username, password string) *Client {
	emailOptions := &TwilioEmailOptions{Username: username, Password: password, Endpoint: "/v3/mail/send"}
	return &Client{emailOptions: *emailOptions}
}

Checklist

• I acknowledge that all my contributions will be made under the project's license
• I have made a material change to the repo (functionality, testing, spelling, grammar)
• I have read the Contribution Guidelines and my PR follows them
• I have titled the PR appropriately
• I have updated my branch with the main branch
• I have added tests that prove my fix is effective or that my feature works
• I have added the necessary documentation about the functionality in the appropriate .md file
• I have added inline documentation to the code I modified

[YaroslavPodorvanov]

This PR will fix #474

[alexandear]

It would be better if the PR's title is shorter.

[alexandear]

This change breaks backward compatibility.

[andrew-pavlov-ua]

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.

[alexandear]

It looks like the maintainers are not concerned with keeping backward compatibility and do not adhere to semver.org. For example, the function SetHost was 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.

[alexandear]

Maybe it's a good idea to add the -race flag to the CI so it gets tested every time:

go test ./... -v -race

Yeah, it's not the topic of the PR. But it will prove changes without cloning them locally and running go test with -race.

[vtopc]

Better to use Mailgun - https://github.com/mailgun/mailgun-go

[andrew-pavlov-ua]

Maybe it's a good idea to add the -race flag to the CI so it gets tested every time:

go test ./... -v -race

Yeah, it's not the topic of the PR. But it will prove changes without cloning them locally and running go test with -race.

Maybe it's a good idea to add the -race flag to the CI so it gets tested every time:

go test ./... -v -race

Yeah, it's not the topic of the PR. But it will prove changes without cloning them locally and running go test with -race.

The -race flag is already set in the test start. Please check the pull request.

[alexandear]

The -race flag is already set in the test start. Please check the pull request.

I carefully reviewed the changes in the pull request and did not find any instances where the -race flag is added to go test.

Allow me to explain my proposal more clearly. I suggest modifying this line in the go.coverage.sh:

sendgrid-go/go.coverage.sh

Line 7 in 16f25e4

go test -coverprofile=profile.out -covermode=atomic "$d"

to:

go test -race -coverprofile=profile.out -covermode=atomic "$d" 

Additionally, I found that in 2018, someone removed the -race flag that was previously present in this commit: e6c23d8. The reason given was that "given the way we present output, we are redundantly running tests with -race, which will make tests run unnecessarily longer, especially for older versions of Go." However, I believe this concern may no longer be relevant for modern versions of Go.

Thank you for considering this suggestion.

[andrew-pavlov-ua]

The -race flag is already set in the test start. Please check the pull request.

I carefully reviewed the changes in the pull request and did not find any instances where the -race flag is added to go test.

Allow me to explain my proposal more clearly. I suggest modifying this line in the go.coverage.sh:

sendgrid-go/go.coverage.sh

Line 7 in 16f25e4

go test -coverprofile=profile.out -covermode=atomic "$d"

to:

go test -race -coverprofile=profile.out -covermode=atomic "$d" 

Additionally, I found that in 2018, someone removed the -race flag that was previously present in this commit: e6c23d8. The reason given was that "given the way we present output, we are redundantly running tests with -race, which will make tests run unnecessarily longer, especially for older versions of Go." However, I believe this concern may no longer be relevant for modern versions of Go.

Thank you for considering this suggestion.

Now I understand your point. It is necessary to add the -race flag in go.coverage.sh.