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

[Adam Domurad]

On 02/11/2013 01:03 PM, Adam Domurad wrote:
> On 02/07/2013 06:04 PM, Omair Majid wrote:
>> On 02/07/2013 05:58 PM, Adam Domurad wrote:
>>> On 02/07/2013 05:10 PM, Omair Majid wrote:
>>>> On 02/07/2013 04:45 PM, Adam Domurad wrote:
>>>>> Tiny patch here.
>>>>>
>>>>> This issue still exists in head:
>>>>> http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=835
>>>>>
>>>>> The patch in the bug report works, although I have preferred the 
>>>>> use of
>>>>> a finally block (I prefer the stronger guarantee).
>>>>>
>>>>> ChangeLog:
>>>>>
>>>>> 2013-XX-XX  Adam Domurad  <adomurad at redhat.com>
>>>>>
>>>>>       * netx/net/sourceforge/jnlp/cache/ResourceTracker.java
>>>>>       (findBestUrl): Ensure that connections are not leaked.
>>>> Looks okay to me.
>>>>
>>>> As an improvement, maybe we can avoid closing and return the "best"
>>>> connection so it can be reused without having to establish another 
>>>> one.
>>>>
>>>> Cheers,
>>>> Omair
>>>>
>>> Good idea. Luckily the code was set-up to make this fairly easy.
>>>
>>> Due to rearranging the disconnect is placed slightly different (more
>>> aligned with the original patch).
>>>
>>> Note that if connection == null, the NPE will get caught. I didn't have
>>> an idea of a better exception to throw, but I figured this should be
>>> noted (it had caused me pains before).
>> Looks great.
>>
>> Cheers,
>> Omair
>>
>
> 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.
>
> This new patch instead takes the route of first trying a HEAD request, 
> which will prevent the entire file being sent (causing a connection to 
> stick around). If it receives 501 == not-implemented, it attempts a 
> GET request with equivalent behaviour to before.
>
> ChangeLog
> 2013-XX-XX  Adam Domurad  <adomurad at redhat.com>
>     PR835: javaws leaks connections
>     Uses HEAD requests if possible for testing URLs. Adds name to 
> download
>     threads.
>     * netx/net/sourceforge/jnlp/cache/ResourceTracker.java:
>     (startThread): Add name to download thread
>     (getResourceUrlResponseCode): Get or fake an HTTP response code.
>     (findBestUrl): Use getResourceUrlResponseCode to first try a HEAD
>     request. Fall-back to GET rquest.
>
> I have bundled a small change to give a name to the downloader threads.
>
> Happy hacking,
> -Adam
>

Seems I have found a server that does not support HEAD requests -- the 
one temporarily set up by the reproducer system :-)
This very simple patch will handle HEAD requests well enough. The patch 
itself has worked with any source I've tried.
After this patch the test system behaves properly with the patch.

Happy hacking,
-Adam

-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR835-tests-handle-HEAD.patch
Type: text/x-patch
Size: 714 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130212/711f3d8c/PR835-tests-handle-HEAD.patch