AsyncHttpClient / async-http-client

Asynchronous Http and WebSocket Client library for Java
Other
6.29k stars 1.59k forks source link

com.ning.http.client.Body.close() Method is Not Called for FileBodyGenerator #896

Closed ghost closed 9 years ago

ghost commented 9 years ago

Steps to Reproduce

./gradlew clean test

For InputStreamBodyGenerator everything is fine.

Environment

com.ning:async-http-client:1.9.25
slandelle commented 9 years ago

@pete-experimenter It looks that you don't provide any provider (neither Netty not Grizzly), so you're falling back to the JDK provider, which is just a toy, isn't maintained, and which will be going away in AHC2.

Please try with a real provider.

Closing for now. Feel free to ping if you can reproduce with Netty or Grizzly.

ghost commented 9 years ago

@slandelle, that was the quickest ticket resolution ever! :+1: Jokes aside I have committed a fix to configure client using Netty. Could you have a look? As it is still reproducible.

slandelle commented 9 years ago

I guess this should be moving in the javadoc.

ghost commented 9 years ago

I have used Grizzly generator and exactly the same behaviour. Please checkout the latest code - it uses both Netty & Grizzly. At this point seems too brave to say in javadoc that Grizzly & Netty don't use it, but FileGenerator is still provided. What's the alternative?

Also would appreciate if you could add to README.md that the below dependencies need to be added to be able to use Grizzly provider. Don't think it would hurt anybody :)

    compile 'org.glassfish.grizzly:grizzly-core:2.3.21'
    compile 'org.glassfish.grizzly:grizzly-http:2.3.21'
    compile 'org.glassfish.grizzly:grizzly-websockets:2.3.21'
    compile 'org.glassfish.grizzly:connection-pool:2.3.21'
slandelle commented 9 years ago

I improved the javadoc in 1.9 to explain Netty provider's behavior and cleaned up class in master.

ghost commented 9 years ago

@slandelle, thanks but taking into account what I have said before, what is the workaround?

slandelle commented 9 years ago

@pete-experimenter The provided BodyGenerator implementations final in master should have been final. How they work are implementation details and the only guaranteed behavior is the one of the interface.

If you want to delete your file when upload is succeeded, you should be doing that either in the Handler's onComplete, or register a listener on the Future.

ghost commented 9 years ago

@slandelle, my personal view is that providing an implementation that does work properly with any of the providers and saying "final" twice plus referring to some implementation to explain that is not a good resolution to me, but if you consider that's how it should be, then there is nothing I can do.

slandelle commented 9 years ago

@pete-experimenter The real issue is that those classes were not meant for extension and should have been final in the first place.

ghost commented 9 years ago

Deprecate the classes then, as they don't work properly and are misleading. I have GB's of data left in temporary folder because of that.

ghost commented 9 years ago

Are you going to update the README.md as I have commented above?

Also would appreciate if you could add to README.md that the below dependencies need to be added to be able to use Grizzly provider. Don't think it would hurt anybody :)

    compile 'org.glassfish.grizzly:grizzly-core:2.3.21'
    compile 'org.glassfish.grizzly:grizzly-http:2.3.21'
    compile 'org.glassfish.grizzly:grizzly-websockets:2.3.21'
    compile 'org.glassfish.grizzly:connection-pool:2.3.21'
slandelle commented 9 years ago

Can't deprecate because passing a FileBodyGenerator is the only way to upload a File with SimpleAsyncHttpClient.

And can't make final in 1.9 without risking breaking existing code.

ghost commented 9 years ago

Right, then I guess https://github.com/AsyncHttpClient/async-http-client/commit/7ce989b04d5f35693c6d54e3f17597dad28c4e15 is good enough. Thanks for your help.

slandelle commented 9 years ago

I'm going to update the README.

Please understand that I inherited this project and spend quite some time filling holes everywhere, and that I only maintain the Netty provider, not the Grizzly one.

ghost commented 9 years ago

Yep, understood. No worries. It is an excellent project I must admit, so thanks for it. Also the updates are so frequent, which is so rare. So, a top notch project all in all!

slandelle commented 9 years ago

Thanks for your kind words. README improved, I hope it will prevent other users to run into the same pitfalls.

ghost commented 9 years ago

Thanks!