Web.Deliver[Async] not Disposing connections

[gatesvp]

Both the Deliver and DeliverAsync methods declare an HttpClient but do not leverage a using() block of have any code to actively close the connection. This leaves the connection hanging.

.NET limits the number of connections to a given outbound server, so with no clean-up it effectively render SendGrid "unresponsive" from that server.

This is likely the cause of issue #93.

[brandonmwest]

Thank you! I'll get this sorted out soon.

[brandonmwest]

The general consensus I've found is that connections are automatically reused and therefore explicitly disposing them hinders performance rather than helps it. From Darrel Miller's blog:

I do not recommend creating a HttpClient inside a Using block to make a single request. When HttpClient is disposed it causes the underlying connection to be closed also. This means the next request has to re-open that connection. You should try and re-use your HttpClient instances. If the server really does not want you holding open it’s connection then it will send a header to request the connection be closed.

And from Henrik Nielsen, one of the principal engineers of HttpClient:

"The default HttpClient is the simplest way in which you can start sending requests. A single HttpClient can be used to send as many HTTP requests as you want concurrently so in many scenarios you can just create one HttpClient and then use that for all your requests.

What am I missing here?

[brandonmwest]

I now think the cause of degraded performance during concurrent calls might be a result of the default connection limit specified by ServicePoint: https://msdn.microsoft.com/en-us/library/system.net.servicepointmanager.defaultconnectionlimit.aspx That limit appears to be dependent on environment in some instances.

This might be causing the connection pool to lock.

[gatesvp]

"You should try and re-use your HttpClient instances"

You are clearly not doing this. That would require pooling of some sort.
Your code is making a new one every time, so there is reuse possible.

You instantiate the HttpClient and then you let it fall out of scope. But
in the background that HttpClient is holding the underlying unmanaged
resources. It can't be garbage collected until Dispose() is called or the
connection times out.

The whole IDisposable pattern is basically there to represent the fact that
there are other resources being held and that explicit cleanup is required.

I had to patch our production servers with a change and the Dispose()
clearly works.

I know about the configuration setting, we already have this set in our
application. But setting it to "infinity" is not really a great solution.
On Apr 22, 2015 6:51 AM, "Brandon West" notifications@github.com wrote:

I now think the cause of degraded performance during concurrent calls
might be a result of the default connection limit specified by
ServicePoint:
https://msdn.microsoft.com/en-us/library/system.net.servicepointmanager.defaultconnectionlimit.aspx

This is causing the connection pool to lock.

—
Reply to this email directly or view it on GitHub
#118 (comment)
.

[brandonmwest]

Your solution does not re-use the connection either, is closing the connection every time. I don't doubt that it works but I want to leave the connection open and re-use it, not dispose of it after each request. You say "that would require pooling of some sort." But each instance of HttpClient has an implicit pool:

The HttpClient class instance acts as a session to send HTTP requests. An HttpClient instance is a collection of settings applied to all requests executed by that instance. In addition, every HttpClient instance uses its own connection pool, isolating its requests from requests executed by other HttpClient instances.

I will work on defining the client outside of the scope of the deliver() call and re-using it, but I will not dispose the object explicity.

[brandonmwest]

Thanks for your help with this. The next nuget release (code is in master) will reuse the connection.

[gatesvp]

This bug still exists in the latest version of the code.

On this line the HttpClient is being instantiated in the constructor, but it is never Disposed.
https://github.com/sendgrid/sendgrid-csharp/blob/master/SendGrid/SendGridMail/Transport/Web.cs#L44

On top of this, the Web class now holds a reference to am IDisposable object but doesn't implement the interface. This lights up all of the static analysis tools. Running Visual Studio's built-in "Code Analysis" or DevExpress/Resharper all point to this as an issue.

So a Web object cannot be disposed and the HttpClient is not disposed which means the connections leak is still present. I would not call this issue fixed at all.

[gatesvp]

As a note the SendGridMessage class has the same problem. It hold a MailMessage that is never Disposed.

[brandonmwest]

I will fix the interface implementations, but I have clearly stated that I will not dispose the HttpClient explicitly, therefore I closed the issue. I'm not disposing the connections on purpose. Everything I've read indicates that it should not be done. For example, http://stackoverflow.com/questions/15705092/do-httpclient-and-httpclienthandler-have-to-be-disposed

[gatesvp]

From that very answer:

The HttpClient object is intended to live for as long as your application needs to make HTTP requests...

However, you're not doing this. You keep creating new ones every time someone instantiates the Web class.

Further down in the comments:

My takeout is that your answer is that the recommended usage pattern is to avoid repeatedly creating and disposing HttpClient instances, but to reuse the same instance throughout the application's lifecycle? ... Yes. If for some reason you do repeatedly create and destroy HttpClient instances then yes, you should Dispose it. I'm not suggesting to ignore the IDisposable interface, just trying to encourage people to re-use instances.

Again, you are not re-using the instance. If I have 25 threads and they all create an instance of Web (it's not a static), then they each get a new instance of HttpClient that is never re-used and never cleaned up.

If I had to be pinned to giving general advice that worked in most (I never say all) cases, I would suggest that you use an IoC container and register an instance of HttpClient as a singleton.

The StackOverflow answer you point to is very nuanced. They're not arguing that you should never Dispose of the HttpClient, instead they are arguing that you should do your best to re-use an existing one instead of constantly disposing and re-creating.

But the current code is doing neither. It's not re-using the HttpClient and it's not disposing. Moreover, it's not letting callers of the class do either of those things.

[MrJoy]

Wait. This code creates -- but does not explicitly reuse -- HttpClient instances? Is the assumption that there's a magical connection pool "behind the scenes", because that's simply not the case. HttpClient keeps the connection open, but it is the responsibility of the code allocating and managing the HttpClient to pool and reuse HttpClient instances.

Note to self: Don't try to use SendGrid from .Net.

[christophermancini]

👍 @gatesvp & @MrJoy

You have to either clean up the connections or reuse them, this is pretty bad architecture to just leave them allocated and abandoned.

[thinkingserious]

Thanks to all for putting in their comments and suggestions!

I'm not sure when we'll have time to dig in, but we will as soon as we can. Any additional advice on how to proceed is much appreciated, since .NET/C# is not my primary platform (I already see some good pointers, thanks!).

A pull request would even be more awesome ;)

[Jericho]

I just committed an improvement in my fork to address this issue. See PR #211.

The improvement is twofold:

1. the constructor of the api client accepts an optional HttpClient. Callers can therefore provide their own httpclient and reuse it if desired (bonus: this also allows the httpclient to be mocked when unit testing the api client class)
2. httpclient is properly disposed when the api client itself is disposed EXCEPT if caller provided it to the constructor. In this scenario, the caller is responsible for disposing the httpclient when appropriate.

[thinkingserious]

Nice work @Jericho!