[rfc][icedtea-web] PR835: javaws findBestUrl leaks TCP connections

[Adam Domurad]

On 02/13/2013 05:37 PM, Omair Majid wrote:
> Hi Adam,
>
> On 02/11/2013 01:03 PM, Adam Domurad wrote:
>> Turns out it wasn't so great :-/
>> Turns out properly reusing HttpURLConnection is quite confusing :-/.
>>
>> OK apparently disconnect calls should be avoided. See David Lloyd's
>> comment @ http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=835.
> Thanks for the pointer.
>
>> +            if (finalLocation == null) {
>> +                System.err.println("Attempted to download " + resource.location + ", but failed to connect!");
>> +            }
>> +
> I am not sure if this is trying to handle finalLocation == null case or
> just an informative message. If this is trying to handle finalLocation
> being null, it doesn't look complete. A NullPointerException will
> probably be thrown a few lines after:

It's just information that gets consumed otherwise. Added an explicit throw.

>
>>               URLConnection connection = finalLocation.openConnection(); // this won't change so should be okay unsynchronized
>> +    private static int getResourceUrlResponseCode(URL url, String requestMethod) throws IOException {
>> +        URLConnection connection = url.openConnection();
>> +        connection.addRequestProperty("Accept-Encoding", "pack200-gzip, gzip");
>> +
> One nit: this method has no information about encodings, but proceeds to
> add the Accept-Encoding header. This looks out of place and mixes
> concerns. Maybe you can modify this method to take the request
> properties? Or maybe this method can take a just-opened URLConnection?

The idea was having 'Resource' in the name signified this, but agreed it 
was weak. OK, dropped 'Resource from name, and accepts a map now.

>
>> +            httpConnection.getInputStream().close();
> It's not clear to me from the docs that this reads/consumes the input. I
> assume you have verified this.

I was trying to avoid consuming input actually because it'd cause a 
pause while the whole get request was consumed. Consuming wasn't needed 
with HEAD requests. Anyway in the interest of correctness I made it 
consume the input.

>
>> +                if (responseCode == 501 /* 501 response signifies Not-Implemented */) {
> Maybe compare responseCode with
> java.net.HttpURLConnection.HTTP_NOT_IMPLEMENTED ?

Good idea, done.

>
> Cheers,
> Omair
>


Thanks,
-Adam
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR835-revised.patch
Type: text/x-patch
Size: 7061 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130214/c59da7f2/PR835-revised.patch