[8u] PING: RFR: 8213734: SAXParser.parse(File, ..) does not close resources when Exception occurs.

[Andrew Hughes]

On 11/10/2019 15:54, Severin Gehwolf wrote:
> On Fri, 2019-10-11 at 16:47 +0200, Mario Torre wrote:
>> Hi Severin,
>>
>> The patch looks good to me.
> 
> Thanks for the review, Mario!
> 
> Cheers,
> Severin
> 
>> On Fri, Sep 27, 2019 at 10:38 AM Severin Gehwolf <sgehwolf at redhat.com> wrote:
>>> Hi!
>>>
>>> I'd appreciate a review of this one. Thanks in advance! See below.
>>>
>>> On Tue, 2019-09-03 at 17:50 +0200, Severin Gehwolf wrote:
>>>> On Mon, 2019-08-26 at 17:54 +0200, Severin Gehwolf wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this 8u backport. The OpenJDK 11u patch does not apply
>>>>> cleanly after path-reshuffeling due to a) test and code for jaxp are
>>>>> split in JDK 8 b) Some rejects in comments - copyright and last
>>>>> modified date. I've omitted rejected comments. I can manually add them
>>>>> if that's preferred. See below.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8213734
>>>>> webrevs:
>>>>>   jdk:  http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8213734/jdk8/jdk/01/webrev/
>>>>>   jaxp: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8213734/jdk8/jaxp/01/webrev/
>>>>>
>>>>> Testing: tier1 on Linux x86_64. javax/xml/jaxp tests. New test on
>>>>>          Windows without the fix fails and passes with it.
>>>>>
>>>>> Thanks to Simon Tooke who helped testing this patch on Windows.
>>>>>
>>>>> Rejects:
>>>>> $ cat src/com/sun/org/apache/xerces/internal/impl/XMLEntityManager.java.rej
>>>>> --- XMLEntityManager.java
>>>>> +++ XMLEntityManager.java
>>>>> @@ -1,5 +1,5 @@
>>>>>  /*
>>>>> - * Copyright (c) 2009, 2017, Oracle and/or its affiliates. All rights reserved.
>>>>> + * Copyright (c) 2009, 2018, Oracle and/or its affiliates. All rights reserved.
>>>>>   */
>>>>>  /*
>>>>>   * Licensed to the Apache Software Foundation (ASF) under one or more
>>>>> @@ -89,7 +89,7 @@
>>>>>   * @author K.Venugopal SUN Microsystems
>>>>>   * @author Neeraj Bajaj SUN Microsystems
>>>>>   * @author Sunitha Reddy SUN Microsystems
>>>>> - * @LastModified: Oct 2017
>>>>> + * @LastModified: Nov 2018
>>>>>   */
>>>>>  public class XMLEntityManager implements XMLComponent, XMLEntityResolver {
>>>>>
>>>>
>>>> Gentle reminder.
>>>
>>> Anyone? It's been a month :(
>>>
>>> FWIW, I've updated XMLEntityManager to include the copyright update
>>> upper bound to 2018. The @LastModified lines don't exist in JDK 8u, so
>>> I've omitted that. Latest webrevs:
>>>
>>> webrevs:
>>>    jdk:  http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8213734/jdk8/jdk/01/webrev/
>>>    jaxp: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8213734/jdk8/jaxp/02/webrev/
>>>
>>> Thanks,
>>> Severin
>>>
>>
>>
> 

The copyright header was going to be my one comment about the JAXP
patch, so good to see it fixed in the second revision. The @LastModified
lines were added by "8193568: @LastModified tag in license header", an
odd fix which is apparently not even viewable. Maybe someone will end up
having to backport it just because it becomes such a pain :(

With the JDK side, you don't really explain why it was necessary to
convert the test from a TestNG one. Could you elaborate please?

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222