sargue / mailgun

Java library to easily send emails using the Mailgun service
MIT License
153 stars 40 forks source link

Resource leak by not closing the client #32

Closed ben-manes closed 4 years ago

ben-manes commented 4 years ago

The HTTP client is not explicitly closed, leading to possible leaks. The JAX-RS ClientBuilder is dynamically resolved, which in my case uses RestEasy instead of Jersey. In that case it uses a finalizer to log a warning that the client wasn't closed, which is a requirement of the API. It would also be nice if these were pooled.

These issues makes it a little uncomfortable to use this SDK in a production with a reasonable email volume.

 * Client is the main entry point to the fluent API used to build and execute client
 * requests in order to consume responses returned.
 * <p/>
 * Clients are heavy-weight objects that manage the client-side communication
 * infrastructure. Initialization as well as disposal of a {@code Client} instance
 * may be a rather expensive operation. It is therefore advised to construct only
 * a small number of {@code Client} instances in the application. Client instances
 * must be {@link #close() properly closed} before being disposed to avoid leaking
 * resources.

https://github.com/sargue/mailgun/blob/abd667ceac159ae0d208b7e1d571cca8959c4bc7/src/main/java/net/sargue/mailgun/Mail.java#L143-L158

sargue commented 4 years ago

You are right. I've been aware of this issue for a while but never faced a problem (low volume of emails). I always end up postponing the solution.

I guess it's time for me to find some time to think of a solution.

sargue commented 4 years ago

By the way, MR are accepted. ;-)

ben-manes commented 4 years ago

Thank you. I decided to write a client in Retrofit instead so that I could use reuse a singleton client with its connection pool, metrics, etc. So this isn't a pressing concern for me, at least.

kopax commented 4 years ago

Hi all, I am looking at this since I want to send email with mailgun and spring boot 1.5.9

Thanks for sharing!

ben-manes commented 4 years ago

@kopax, I was using simpleJavaMail, which is a really excellent library. However, the SMTP endpoint is very flaky at scale and requires retries. Mailgun's support wasn't helpful except acknowledging that they do some nasty things in their load balancer to cycle IPs for defensive reasons. They said the HTTP api was faster and more robust, so I had to port over.

I wrote an Email class similar to @sargue's MailBuilder class to aggregate the contents into a multipart. This goes to a Retrofit interface, which is backed by a shared OkHttp client. Not much work thanks to this library and the api docs to clarify their endpoint.

mailgun.zip

kopax commented 4 years ago

Thanks, @ben-manes for sharing this. I have opened the file and see the whole part. It would be nice to have a GitHub repo for this as well (fork) but that's not a requirement. (I assume you are using the latest version of each class imported in your files).

My requirement are:

I have not done much bulk mail sending so I can't tell if the work is already done here in your zip file, do you think it is a good starting point to start with your achieved work?

Thanks again,

ben-manes commented 4 years ago

My requirements are the same and not bulk mail, but it amounts to around 800k/month. This is for example a company sending an invoice to their customer.

I did not include the code using this client, which adds those to the Email class, invokes the Mailgun.send method, and handles all of the metrics / retrials / etc. You would use this as,

Email email = new Email()
    .from(...).to(...)
    .text("Hello World")
    .attachment(filename, file, mediaType);
mailgun.send(domain, email);

Anyone is welcome to formalize into a reusable client, like @sargue's nice library. It was not much work on my part, Retrofit does all the heavy lifting and this is just an example of how quickly one can bootstrap a usage.

kopax commented 4 years ago

Apparently @sargue fixed the issue there. Thanks a lot for this.

Now that this is fixed, I wonder which one should I use, is there any difference ?

I think OSS has some advantages, @sargue do you have anytought on @ben-manes work ?

ben-manes commented 4 years ago

Thanks @sargue!

I started with @sargue's and would have been happy to use it. Likewise it was little effort to write my own, as I didn't want to create pressure for a fix and continued maintenance (OSS can be exhausting as a never ending responsibility of free labor). There is no technical reason to not use @sargue other than if you desire more API features where mine provides a better starting point thanks to Retrofit.

sargue commented 4 years ago

I haven't checked Ben's work and I don't know Retrofit so I really can't say. You should evaluate your needs and pick was best for you.