Skip to content
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

Instantiate the AsyncHttpClient lazily #118

Closed
wants to merge 1 commit into from

Conversation

cb372
Copy link

@cb372 cb372 commented Aug 25, 2015

This is to fix the resource leak caused by creating a new client in
configure and not shutting down the old one.

Instantiating a singleton client was a problem because (by definition of
being a singleton) it might be shared between multiple Http instances,
meaning it's hard to know when you can/should shut it down.

Got around the problem by making Http hold a config builder and a
lazy val client.

It's still possible to leak an AsyncHttpClient instance by making
requests and then calling configure later, but I think people are less
likely to do that. The normal use case is to configure your client at
application startup and then start making requests.

This is to fix the resource leak caused by creating a new client in
`configure` and not shutting down the old one.

Instantiating a singleton client was a problem because (by definition of
being a singleton) it might be shared between multiple `Http` instances,
meaning it's hard to know when you can/should shut it down.

Got around the problem by making `Http` hold a config builder and a
`lazy val client`.

It's still possible to leak an `AsyncHttpClient` instance by making
requests and then calling `configure` later, but I think people are less
likely to do that. The normal use case is to configure your client at
application startup and then start making requests.
@cb372
Copy link
Author

cb372 commented Aug 25, 2015

As far as I can tell, this supersedes PR #115 and fixes #114 #112 #99 #59 #53

@cb372
Copy link
Author

cb372 commented Aug 25, 2015

Not sure why Travis failed. Tests pass on my machine.

@davidshen84
Copy link

I checked the your Travis log, everything looks good. I think it is a Travis issue. I saw there are a few Travis related commits. Could you please rebase your commits on those commits, and try again?

This hidden instance is really bugging me.

@cb372
Copy link
Author

cb372 commented Aug 22, 2016

I could rebase, but there has been no activity on this repo for a year. I think this project is dead.

@davidshen84
Copy link

Err~I kinda sense that. But if you google "scala async http client",
dispatch is still the top one. What do scala folks use for async http
client now? I think I am out of date in the scala community for a while.

On Mon, Aug 22, 2016 at 9:40 PM Chris Birchall notifications@github.com
wrote:

I could rebase, but there has been no activity on this repo for a year. I
think this project is dead.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#118 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEAzf5KiGI6AnKR4-1FJMnrOhmy9MIrks5qiabJgaJpZM4Fxybg
.

Thanks,
David S.

@mateusduboli
Copy link

Is this going to be merged?

@ashawley
Copy link
Contributor

ashawley commented Dec 5, 2016

@mateusduboli Are you needing this patch? It's fixing a bug in 0.11.3, but isn't a bug in version 0.11.2.

@mateusduboli
Copy link

@ashawley I compared the two versions and the only thing that could be pointed as a relevant reason for this to not be present in 0.11.2 is the version of AsyncHttpClient, which was 1.8.10.

Is that the problem that causes the leakage of clients?

@ashawley
Copy link
Contributor

ashawley commented Dec 6, 2016

@mateusduboli Yes, exactly. Here was the discussion on the dispatch mailing list
https://groups.google.com/forum/#!msg/dispatch-scala/iy12POc81LM/udzLH14ajcMJ

@fbascheper
Copy link
Contributor

LGTM. I've cherry-picked this into pull request #139 .

@ashawley
Copy link
Contributor

This will fix 0.11.3 and 0.12.0 as well as fixing 0.13.

@fbascheper
Copy link
Contributor

@farmdawgnation AFAIK this is now merged / superseded by the AHC 2.0 update.

@farmdawgnation
Copy link
Member

Acknowledged, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants