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

Wed Mar 9 19:36:18 UTC 2016

The fix/workaround itself looks fine. My concern is that even the native
inflater does increase its usage a little, but it seems like the "real" root
cause is that someone's code (out of the core-libs) uses the inflater without
releasing it AND the finalizer is not faster enough to finalize/release them.
Now we have to put a patch/workaround in the Inflater to workaround
that issue...

-Sherman

On 03/03/2016 05:34 AM, Andrew Brygin wrote:
> I’d like to cast a vote for inclusion this fix in jdk9.
>
> Probably it has to be done in the original review thread created by Alex:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036463.html
> http://mail.openjdk.java.net/pipermail/jdk9-dev/2015-November/003036.html
> but there is no any activity from November 2015…
>
> So, +1 to get this fix in jdk9.
>
> Thanks,
> Andrew
>
>
>> On Feb 24, 2016, at 5:07 PM, Dmitry Cherepanov <dcherepanov at azul.com <mailto:dcherepanov at azul.com>> wrote:
>>
>>
>>> On Feb 19, 2016, at 1:55 PM, Nikolay Gorshkov <nikolay at azulsystems.com <mailto:nikolay at azulsystems.com>> wrote:
>>>
>>> Hi Sherman, Sean,
>>>
>>> Could you please help with making progress on this code review request?
>>> This fix is waiting for review since October.
>>>
>>> Webrev for jdk7u:
>>> http://cr.openjdk.java.net/~nikgor/8133206/jdk7u-dev/webrev.01/ <http://cr.openjdk.java.net/%7Enikgor/8133206/jdk7u-dev/webrev.01/>
>>> Original mail thread:
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-October/035884.html
>>
>> I’m not expert in this area but the changes look reasonable to me. +1 for pushing this into JDK9.
>>
>> Thanks
>>
>> Dmitry
>>
>>>
>>> Webrev for jdk9 (contributed by Alex Kashchenko):
>>> http://cr.openjdk.java.net/~akasko/jdk9/8133206/webrev.00/ <http://cr.openjdk.java.net/%7Eakasko/jdk9/8133206/webrev.00/>
>>> Original mail thread:
>>> http://mail.openjdk.java.net/pipermail/jdk9-dev/2015-November/003036.html
>>>
>>> Thanks,
>>> Nikolay
>>>
>>> On 11.12.2015 15:09, Nikolay Gorshkov wrote:
>>>> Hi Sean,
>>>>
>>>> Thank you for your attention to this matter!
>>>> Actually, the code review request was sent to core-libs-dev alias a month ago:
>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036463.html
>>>> Unfortunately, we haven't got any feedback yet.
>>>>
>>>> Thanks,
>>>> Nikolay
>>>>
>>>> On 11.12.2015 14:34, Seán Coffey wrote:
>>>>> 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
>>>>>>
>>>>>
>>
>