RFR [7] 8133206: "java.lang.OutOfMemoryError: unable to create new native thread" caused by upgrade to zlib 1.2.8

[Seán Coffey]

Hi Alex,

I'm dropping jdk7u-dev mailing list for the moment. core-libs-dev is the 
mailing list where this review should happen. This fix would be required 
in JDK 9 first as per process. I think Sherman would be best to review 
if possible.

Once it's soaked in JDK 9 for a few weeks, we could consider jdk8u and 
7u backports.

Regards,
Sean.

On 10/11/15 10:57, Alex Kashchenko wrote:
> Hi,
>
> On 10/21/2015 04:39 PM, Nikolay Gorshkov wrote:
>> Hi Sherman,
>>
>> Thank you for your reply! My answers are inlined.
>>
>>  > Can you be more specific about the "class loading cases" above? 
>> Sounds
>>  > more like we have a memory leaking here (the real root cause)? for
>> example
>>  > the inflateEnd() never gets called?
>>
>> I agree, the real root cause is probably the following issue that exists
>> since the end of 2002:
>> https://bugs.openjdk.java.net/browse/JDK-4797189
>> "Finalizers not called promptly enough"
>> And it is "the absence of a general solution to the non-heap resource
>> exhaustion problem".
>>
>> zlib's inflateEnd() function is called by
>> void java.util.zip.Inflater.end(long addr)
>> native method only, and this method, in turn, is called only by
>> void java.util.zip.Inflater.end()
>> and
>> void java.util.zip.Inflater.finalize()
>> methods. According to the experiments, the typical stack trace for
>> instantiating java.util.zip.Inflater is:
>>
>> java.util.zip.Inflater.<init>(Inflater.java:116)
>> java.util.zip.ZipFile.getInflater(ZipFile.java:450)
>> java.util.zip.ZipFile.getInputStream(ZipFile.java:369)
>> java.util.jar.JarFile.getInputStream(JarFile.java:412)
>> org.jboss.virtual.plugins.context.zip.ZipFileWrapper.openStream(ZipFileWrapper.java:222) 
>>
>>
>>    <more jboss frames>
>> org.jboss.classloader.spi.base.BaseClassLoader$2.run(BaseClassLoader.java:592) 
>>
>>
>> java.security.AccessController.doPrivileged(Native Method)
>> org.jboss.classloader.spi.base.BaseClassLoader.loadClassLocally(BaseClassLoader.java:591) 
>>
>>
>>    <more jboss frames>
>> org.jboss.classloader.spi.base.BaseClassLoader.loadClass(BaseClassLoader.java:447) 
>>
>>
>> java.lang.ClassLoader.loadClass(ClassLoader.java:358)
>> java.lang.Class.forName0(Native Method)
>> java.lang.Class.forName(Class.java:278)
>> org.jboss.deployers.plugins.annotations.WeakClassLoaderHolder.loadClass(WeakClassLoaderHolder.java:72) 
>>
>>
>>    <more jboss and other frames>
>>
>> It's quite hard to understand who is responsible for not calling
>> Inflater.end()
>> method explicitly; probably, it is the jboss/application's code.
>> Unfortunately,
>> we were in "it worked before and is broken now" customer situation 
>> here, so
>> needed to fix it anyway.
>>
>>  >  From the doc/impl in inflate() it appears the proposed change 
>> should be
>>  > fine, though it's a little hacky, as you never know if it starts to
>> return
>>  > Z_OK from some future release(s). Since the "current" implementation
>>  > never returns Z_OK, it might be worth considering to keep the Z_OK 
>> logic
>>  > asis in Inflater.c, together with the Z_BUF_ERROR, just in case?
>>
>> OK, I added handling of Z_OK code back.
>>
>>  > I would be desired to add some words in Inflater.c to remind the
>>  > future maintainer why we switched from partial to finish and why to
>>  > check z_buf_error.
>>
>> I agree, added a comment.
>>
>> The updated webrev is available here:
>>
>> http://cr.openjdk.java.net/~nikgor/8133206/jdk7u-dev/webrev.01/
>>
>
> The change looks good to me (not a Reviewer/Committer).
>
> Patched jdk7u also passes JCK-7 on RHEL 7.1.
>
> I forward-ported this patch to jdk9 (consulted with Nikolay Gorshkov 
> first), jtreg reproducer for jdk9 also works with jdk7u - 
> http://mail.openjdk.java.net/pipermail/jdk9-dev/2015-November/003036.html
>