sendgrid-csharp icon indicating copy to clipboard operation
sendgrid-csharp copied to clipboard

SendGridClient thread-safety documentation

Open william-gross opened this issue 8 years ago • 24 comments

Please state in the documentation whether it is safe to create a single SendGridClient object and share it among all threads in an app domain. In particular, I'd like to know if it is safe for multiple threads in an ASP.NET application to call SendEmailAsync or RequestAsync at the same time.

I believe this was safe in version 6.3.4 (with SendGrid.Web.DeliverAsync) and it's probably still safe now (since the underlying implementation is HttpClient), but it would be nice to know for sure.

william-gross avatar Sep 12 '17 19:09 william-gross

Hello @william-gross,

While I have not done any specific testing for thread safety, I believe your assumption is correct with regards to [HttpClient](https://msdn.microsoft.com/en-us/library/system.net.http.httpclient(v=vs.110).aspx#Anchor_5).

I will leave this thread open so that we can create a test that verifies and documents thread safety with a real world example.

For this issue to gain priority in our backlog, we need additional +1's or a PR. When we receive a PR, that provides the biggest jump in priority.

With Best Regards,

Elmer

thinkingserious avatar Sep 12 '17 20:09 thinkingserious

I'd like to know this as well for DI purposes.

paultechguy avatar Oct 03 '17 20:10 paultechguy

Thanks @paultechguy,

I've add your vote to our backlog.

thinkingserious avatar Oct 04 '17 01:10 thinkingserious

I would also like to have this confirmed.

winzig avatar Feb 20 '18 04:02 winzig

Thanks for the upvote @winzig.

thinkingserious avatar Feb 22 '18 19:02 thinkingserious

Another Up from me 👍 :)

alexmaie avatar May 18 '18 09:05 alexmaie

Still no confirmation?

profet23 avatar May 29 '18 13:05 profet23

Hi @profet23,

Not yet, but I've added your vote to help this issue rise in priority. Thanks!

With Best Regards,

Elmer

thinkingserious avatar May 30 '18 03:05 thinkingserious

Please :+1:

kierenj avatar Aug 14 '18 11:08 kierenj

Thanks for the vote @kierenj!

thinkingserious avatar Aug 14 '18 18:08 thinkingserious

Up

twomm avatar Nov 05 '18 10:11 twomm

@thinkingserious Could you please update us on the issue?

MSC-AA avatar Nov 28 '18 14:11 MSC-AA

Also a vote from me, etc! Can we Singleton or Scope this? etc..

PureKrome avatar Jan 10 '19 04:01 PureKrome

Hello @MSC-AA,

This one is still on our backlog. We are hoping to increase our resources internally soon to help us power through our backlog faster. If you know anyone, please let us know at [email protected].

Thanks for checking in!

With Best Regards,

Elmer

thinkingserious avatar Jan 17 '19 20:01 thinkingserious

Referencing: https://github.com/sendgrid/sendgrid-csharp/issues/847

thinkingserious avatar Jan 17 '19 20:01 thinkingserious

I want to find out it too, can we use Singleton or not.

rodion-m avatar Feb 18 '19 09:02 rodion-m

Another vote from me!

kntajus avatar Mar 22 '19 12:03 kntajus

Another vote from me!

biliktamas79 avatar Jul 04 '19 12:07 biliktamas79

Me too!

CleytonGoncalves avatar Sep 13 '19 14:09 CleytonGoncalves

It's very strange that SendGrid does not know if their own client is thread safe or not...

This is important details, e.g. Microsoft's DbContext is not thread safe, so we need to instantiate a new instance every time we need it, on the other hand Elasticsearch client is thread safe, so we need to reuse the same instance for the entire lifetime of the application. Getting this wrong would have severe consequences, for example Elasticsearch does all the caching in the client, so if we keep instantiating a new instance we won't have any cache.

Unfortunately, people who come across this thread would lose confidence in your product...

HoomanBahreini avatar Nov 19 '19 03:11 HoomanBahreini

Given that the SendGridClient class encapsulates a HttpClient instance field, it would seem to me that using it as a singleton would at least potentially have the issues documented with HttpClient singletons:

Another issue that developers run into is when using a shared instance of HttpClient in long running processes. In a situation where the HttpClient is instantiated as a singleton or a static object, it fails to handle the DNS changes as described in this issue of the dotnet/corefx GitHub repository.

So at least from that perspective, it seems wise to use scoped lifetime even if otherwise thread safe?

NoahStahl avatar Mar 09 '20 23:03 NoahStahl

Up and also up for the comment of @HoomanBahreini... The question has been asked 3 years ago and no one is able to say whether SendGrid is thread safe or not o_O That's insane...

ssougnez avatar Aug 08 '20 14:08 ssougnez

@ssougnez I was going to come in here, agree with you AND also suggest that the lack of any response speaks volumes about SG and what they think about .NET Devs (with respect to using/consuming their product).

--BUT-- 4 days go @childish-sambino changes the tags on this issue ... which actually looks like the first type of interaction (on this thread) since Q3 2018 - nearly 2 years ago.

so maybe.... they are interested again? not sure.

It's pretty frustrating, none-the-less.

PureKrome avatar Aug 08 '20 14:08 PureKrome

SendEmailAsync is thread safe.

First thing it does is

 return await this.RequestAsync(
                Method.POST,
                msg.Serialize(),
                urlPath: "mail/send",
                cancellationToken: cancellationToken).ConfigureAwait(false);

Nothing unsafe for threads here. Using only local variables.

Then it is executing method from base class. I assume that fields/props inside this.options does not change. The method contains following operations:

var baseAddress = new Uri(this.options.Host);
            if (!baseAddress.OriginalString.EndsWith("/"))
            {
                baseAddress = new Uri(baseAddress.OriginalString + "/");
            }

Creates local var from readonly object - thread safe

// Build the final request
            var request = new HttpRequestMessage
            {
                Method = new HttpMethod(method.ToString()),
                RequestUri = new Uri(baseAddress, this.BuildUrl(urlPath, queryParams)),
                Content = requestBody == null ? null : new StringContent(requestBody, Encoding.UTF8, this.MediaType),
            };

creates local variable from method params - thread safe.

// Drop the default UTF-8 content type charset for JSON payloads since some APIs may not accept it.
            if (request.Content != null && this.MediaType == DefaultMediaType)
            {
                request.Content.Headers.ContentType.CharSet = null;
            }

            // set header overrides
            if (this.options.RequestHeaders?.Count > 0)
            {
                foreach (var header in this.options.RequestHeaders)
                {
                    request.Headers.TryAddWithoutValidation(header.Key, header.Value);
                }
            }

Does some operations on local object with help of readonly field - thread safe.

  // set standard headers
            request.Headers.Authorization = this.options.Auth;
            request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue(this.MediaType));
            request.Headers.UserAgent.TryParseAdd($"sendgrid/{ClientVersion} csharp");
            return await this.MakeRequest(request, cancellationToken).ConfigureAwait(false);

Thread safe - we are only operating on local object. Let's dive into this.MakeRequest

 HttpResponseMessage response = await this.client.SendAsync(request, cancellationToken).ConfigureAwait(false);

This is thread safe op.

if (!response.IsSuccessStatusCode && this.options.HttpErrorAsException)
            {
                await ErrorHandler.ThrowException(response).ConfigureAwait(false);
            }

using local variable and readonly thingy - thread safe.

/ Correct invalid UTF-8 charset values returned by API
            if (response.Content?.Headers?.ContentType?.CharSet == "utf8")
            {
                response.Content.Headers.ContentType.CharSet = Encoding.UTF8.WebName;
            }

Only local variable - thread safe.

/ Correct invalid UTF-8 charset values returned by API
            if (response.Content?.Headers?.ContentType?.CharSet == "utf8")
            {
                response.Content.Headers.ContentType.CharSet = Encoding.UTF8.WebName;
            }

Only local var - thread safe.


I did not find anything thread unsafe even if you share httpClient (but i suggest using httpclientfactory).

shoter avatar Dec 11 '20 14:12 shoter