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

Exceptions when using together with AsyncHttpClient #87

Closed
domdorn opened this issue Mar 9, 2017 · 11 comments
Closed

Exceptions when using together with AsyncHttpClient #87

domdorn opened this issue Mar 9, 2017 · 11 comments

Comments

@domdorn
Copy link
Member

domdorn commented Mar 9, 2017

Sorry for cross-posting. I've written down the details of this issue at:
playframework/playframework#7056

It looks like (at least in my Play 2.6.0-M1 app) that the shaded AsyncHttpClientDefaults class reads the ahc-default.properties that are pulled in from the non-shaded version on my classpath.

The issue probably comes from
https://github.com/AsyncHttpClient/async-http-client/blob/async-http-client-project-2.0.27/client/src/main/java/org/asynchttpclient/config/AsyncHttpClientConfigHelper.java#L47

Where the current ThreadContext Classloader is used to lookup the properties instead a classloader based on the current class (which would then find the ahc-default.properties with the play.shaded prefix.

@domdorn
Copy link
Member Author

domdorn commented Apr 11, 2017

So what we would need to do is to also rename the ahc-default.properties to something like play.shaded-ahc-default.properties and replace references to it.
I can provide a PR for that if that is OK for you @wsargent / @marcospereira

@domdorn domdorn changed the title shaded AsyncHttpClientDefaults reads wrong file -> default lookup fails (Play 2.6.0-M1) Exceptions when using together with AsyncHttpClient Apr 11, 2017
@wsargent
Copy link
Member

Yes please.

@domdorn
Copy link
Member Author

domdorn commented May 15, 2017

Ok, unfortunately, "jar jar links" only supports changing the package name of the classes transformed. I've managed to rename the ahc-default.properties and ahc.properties files themselves, but unfortunately, references to these are hardcoded (as constants) in
https://github.com/AsyncHttpClient/async-http-client/blob/async-http-client-project-2.0.27/client/src/main/java/org/asynchttpclient/config/AsyncHttpClientConfigHelper.java#L33
respectively in
https://github.com/AsyncHttpClient/async-http-client/blob/async-http-client-project-2.0.27/client/src/main/java/org/asynchttpclient/config/AsyncHttpClientConfigHelper.java#L37 / https://github.com/AsyncHttpClient/async-http-client/blob/async-http-client-project-2.0.27/client/src/main/java/org/asynchttpclient/config/AsyncHttpClientConfigHelper.java#L41

Unfortunately, the whole AysncHttpClientConfigHelper is written in a way that does not really allow outside modification/configuration. We could try to exclude it in the shading process and provide our own version of it, but that feels like a rather bad hack.

@domdorn
Copy link
Member Author

domdorn commented May 15, 2017

A alternative to shading the async-http-client on a class-file level could be to include the async-http-client project as a sbt-source-dependency .. that way it might be possible to basically take the files and rewrite them directly (and thus making also changes to the constants in AsyncHttpClientConfigHelper .. but its another "hack".

@wsargent
Copy link
Member

@wsargent
Copy link
Member

@domdorn do you have a PR for this?

@domdorn
Copy link
Member Author

domdorn commented Jun 22, 2017

@wsargent I tried to adjust the shading process, but the shading library used only allows simple byte-code changes (changing package names) -> I was unable to change the constant.
I once did something similar with cglib, but I'm not sure we want to go down that route.

@domdorn
Copy link
Member Author

domdorn commented Jun 22, 2017

Hmm... I've got an idea... what if instead of replacing the variables in the ahc.properties, we're just adding to them?
so instead of only writing
play.shaded.ahc.org.asynchttpclient.threadPoolName=AsyncHttpClient
to the file, we write

play.shaded.ahc.org.asynchttpclient.threadPoolName=AsyncHttpClient
org.asynchttpclient.threadPoolName=AsyncHttpClient

to the file? That way, we still be providing the default file, and also provide our own properties. People can still change their settings in the other config file. WDYT?

domdorn added a commit that referenced this issue Jun 22, 2017
completely replacing the configuration properties resulted in errors when users
where using the unshaded AHC-Client in their applications together with play-ws.
As a workaround we take the original properties and _add_ our own properties to the
file. This way, the unshaded client still finds its properties while our own version can
be configured independently
domdorn added a commit that referenced this issue Jun 22, 2017
@wsargent
Copy link
Member

Okay, I can address that in a patch release.

@domdorn
Copy link
Member Author

domdorn commented Jun 22, 2017

@wsargent I have a PR for this: #149

wsargent pushed a commit that referenced this issue Jun 27, 2017
* #87 add original + shaded configuration properties

completely replacing the configuration properties resulted in errors when users
where using the unshaded AHC-Client in their applications together with play-ws.
As a workaround we take the original properties and _add_ our own properties to the
file. This way, the unshaded client still finds its properties while our own version can
be configured independently

* #87 add original + shaded configuration properties

forgot to add newline.
@wsargent
Copy link
Member

Closing by way of #149

wsargent pushed a commit to wsargent/play-ws that referenced this issue Jul 5, 2017
completely replacing the configuration properties resulted in errors when users
where using the unshaded AHC-Client in their applications together with play-ws.
As a workaround we take the original properties and _add_ our own properties to the
file. This way, the unshaded client still finds its properties while our own version can
be configured independently
wsargent pushed a commit to wsargent/play-ws that referenced this issue Jul 5, 2017
wsargent pushed a commit that referenced this issue Jul 5, 2017
* #87 add original + shaded configuration properties

completely replacing the configuration properties resulted in errors when users
where using the unshaded AHC-Client in their applications together with play-ws.
As a workaround we take the original properties and _add_ our own properties to the
file. This way, the unshaded client still finds its properties while our own version can
be configured independently

* #87 add original + shaded configuration properties

forgot to add newline.
@marcospereira marcospereira marked this as a duplicate of #177 Jul 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants