search

ning / async-http-client

Asynchronous Http Client for Java
135 stars 50 forks source link

Inconsistent behavior of NettyResponseFuture.get() vs NettyResponseFuture.get(long, TimeUnit) #26

Closed aantono closed 14 years ago

aantono commented 14 years ago

It appears as if a "blocking" .get() method marks the future as done if its not and always checks for the exEx reference to see if it has an exception... Naturally, one would expect the behavior of .get(long, TimeUnit) to be exactly identical, except for the different timeout period, which is, however, not the case. internally the future is never marked as "done" and the exEx checking is done only if the future is not done and not canceled. It seems as if the .get(long, TimeUnit) should behave identically to the .get() with the exception of the latch.await timeout value and time unit.

jfarcand commented 14 years ago

Agree....now let me think why I did that. Looking at it.

jfarcand commented 14 years ago

Fixed

197978a..60eae40 master -> master

Please review if you can. THANKS!

aantono commented 14 years ago

Looks good. One question though, shouldn't the if (exEx.get() != null) { throw exEx.getAndSet(null); } be moved outside of the if block, like it was in the original .get() method?

jfarcand commented 14 years ago

No, that was a bug previously as the exception should only be set once we set the response to done. WDYT?

aantono commented 14 years ago

Yeah, that makes sense. Just wanted to make sure the change was intentional. :)

jfarcand commented 14 years ago

Thanks for reviewing!